bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

bug#66863: [PATCH] Add two docstrings in cl-macs.el


From: Jeremy Bryant
Subject: bug#66863: [PATCH] Add two docstrings in cl-macs.el
Date: Tue, 31 Oct 2023 23:40:04 +0000

Attachment: 0001-Add-two-docstrings-in-cl-macs.el.patch
Description: Text Data

Hi Stefan,

Revised patch attached and comments below

Kind regards
Jeremy Bryant 

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> * lisp/emacs-lisp/cl-macs.el (cl--simple-exprs-p)
>> (cl--const-expr-p): Add docstrings.
>
> Hmm... tackling some tricky ones, eh?

Apparently yes!

>
>>  (defun cl--simple-exprs-p (xs)
>> +  "Check if no side effects, and executes quickly, for each element of list 
>> XS."
>>    (while (and xs (cl--simple-expr-p (car xs)))
>>      (setq xs (cdr xs)))
>>    (not xs))
>
> I think I'd use a ref to `cl--simple-expr-p`, so we don't duplicate the
> poorly defined "no side effects, and executes quickly".

See revised patch which follows this advice

>
> Wait... `grep` says this function is not used any more, so maybe the
> better option is to delete it.

>
>> @@ -118,6 +119,7 @@ cl--safe-expr-p
>>  
>>  ;;; Check if constant (i.e., no side effects or dependencies).
>>  (defun cl--const-expr-p (x)
>> +  "Check if X is constant (i.e., no side effects or dependencies)."
>>    (cond ((consp x)
>>       (or (eq (car x) 'quote)
>>           (and (memq (car x) '(function cl-function))
>
> Yeah..hmm.. by order of increasing preference:
> - Move the comment to the docstring rather that just copying it.

Done

> - Clarify the difference with `macroexp-const-p`.

Added reference to this with my interpretation of the difference (which
could need change or not)

> - Declare the function obsoleted by `macroexp-const-p`.
> - Delete the function altogether.
>
>
>         Stefan


reply via email to

[Prev in Thread] Current Thread [Next in Thread]