Re: [PATCH] Add new lisp function length= with bytecode support

[Eli Zaretskii]

> Date: Sun, 26 Feb 2017 17:04:26 -0500
> From: Gdobbins <address@hidden>
>
> The attatched patch has two commits. The first implements a new lisp 
> function, length=, which acts the same
> as the function of the same name from the Common Lisp package Alexandria. 
> Namely it compares the length
> of sequences with numbers. The second modifies the byte interpreter to treat 
> = the same as length=, and the
> byte compiler to generate the appropriate code. By greping the Emacs repo's 
> *.el files and comparing to *.elc
> files after compiling with the compiler-macro in the second commit of this 
> patch, I estimate 14% of all calls to
> length in Emacs are then simply compared via =. This makes length= quite 
> useful, and since a large amount
> of linked list traversal can be avoided, potentially more efficient.

Thanks.  Please allow me a few comments.

> diff --git a/etc/NEWS b/etc/NEWS
> index 31b7e4789e..6abcda729b 100644
> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -970,6 +970,10 @@ that does not exist.
>  operating recursively and when some other process deletes the directory
>  or its files before 'delete-directory' gets to them.
>  
> ++++
> +** The new function 'length=' compares the lengths of sequences and
> +numbers.

The "+++" marker means that the following entry is already described
in the appropriate manual.  But your changeset doesn't include any
changes for the manuals.  Were they forgotten?

> +DEFUN ("length=", Flength_eqlsign, Slength_eqlsign, 1, MANY, 0,
> +       doc: /* Each element of SEQUENCES may be any type accepted by
> +`length' or `='.  True if the length of each sequence is equal to each
> +number, otherwise returns nil.
> +usage: (length= &rest SEQUENCES) */)
> +  (ptrdiff_t nargs, Lisp_Object *args)

IMO, this doc string needs to be clarified, and will probably benefit
from an example.  E.g., you refer to "number", but the list of
arguments doesn't seem to describe any numbers.  So the semantics of
this function remains unclear after reading the doc string.

> +      if (Fnumber_or_marker_p (args[argnum]))

It would be more efficient to call NUMBERP and MARKERP directly.

> +       else if (! CALLN (Feqlsign, val, args[argnum]))

Not sure why you call Feqlsign here.  The arguments are either numbers
or markers, so their direct comparison should be much more efficient,
right?