GNU bug report logs - #66863
[PATCH] Add two docstrings in cl-macs.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 66863 in the body.
You can then email your comments to 66863 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: bug-gnu-emacs <at> gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: [PATCH] Add two docstrings in cl-macs.el
Date: Tue, 31 Oct 2023 22:36:57 +0000

[Message part 1 (text/plain, inline)]

Tags: patch

Hi Emacs maintainers,

Here is a proposed patch with two docstrings added to cl-macs.el where
there were none previously.  In particular, from previous changes, I
assume that Stefan you may be familiar with this area?  I look forward
to your feedback.

Best
Jeremy 

[0001-Add-two-docstrings-in-cl-macs.el.patch (text/patch, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Add two docstrings in cl-macs.el
Date: Tue, 31 Oct 2023 18:58:26 -0400

> * lisp/emacs-lisp/cl-macs.el (cl--simple-exprs-p)
> (cl--const-expr-p): Add docstrings.

Hmm... tackling some tricky ones, eh?

>  (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".

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.
- Clarify the difference with `macroexp-const-p`.
- Declare the function obsoleted by `macroexp-const-p`.
- Delete the function altogether.


        Stefan

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Add two docstrings in cl-macs.el
Date: Tue, 31 Oct 2023 23:40:04 +0000

[0001-Add-two-docstrings-in-cl-macs.el.patch (text/x-diff, attachment)]

[Message part 2 (text/plain, inline)]

Hi Stefan,

Revised patch attached and comments below

Kind regards
Jeremy Bryant 

Stefan Monnier <monnier <at> 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

Message #14 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jeremy Bryant <jb <at> jeremybryant.net>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Add two docstrings in cl-macs.el
Date: Wed, 01 Nov 2023 08:17:36 -0400

> From 16298965905b1859aa8a342b98803f4abd6da3f0 Mon Sep 17 00:00:00 2001
> From: Jeremy Bryant <jb <at> jeremybryant.net>
> Date: Tue, 31 Oct 2023 23:37:10 +0000
> Subject: [PATCH] Add two docstrings in cl-macs.el

Thanks, pushed.

>  (defun cl--simple-exprs-p (xs)
> +  "Map `cl--simple-expr-p' to each element of list XS."

This doesn't say how the results of `cl--simple-expr-p` are combined,
it might even suggest they're returned as a list.
I think I would have said something like 

    "Like `cl--simple-expr-p' but for a list of expressions."

>  (defun cl--const-expr-p (x)
> +  "Check if X is constant (i.e., no side effects or dependencies).
> +
> +See `macroexp-const-p' for similar functionality without cl-lib dependency."

"Similar" fails to describe the difference :-(
[ Note, I can't blame you: I know the difference but I couldn't come up
  with a useful description of it, that is, one that is easier to
  understand than the code itself.  FWIW I believe the difference is
  a latent bug, which is partly why I'd like to delete the function :-)  ]


        Stefan

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jeremy Bryant <jb <at> jeremybryant.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Add two docstrings in cl-macs.el
Date: Sat, 04 Nov 2023 09:23:42 +0000

Thank you, I've made a note on conventions.  For this patch, as you have
pushed, does this mean this is closed?


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

>> From 16298965905b1859aa8a342b98803f4abd6da3f0 Mon Sep 17 00:00:00 2001
>> From: Jeremy Bryant <jb <at> jeremybryant.net>
>> Date: Tue, 31 Oct 2023 23:37:10 +0000
>> Subject: [PATCH] Add two docstrings in cl-macs.el
>
> Thanks, pushed.
>
>>  (defun cl--simple-exprs-p (xs)
>> +  "Map `cl--simple-expr-p' to each element of list XS."
>
> This doesn't say how the results of `cl--simple-expr-p` are combined,
> it might even suggest they're returned as a list.
> I think I would have said something like 
>
>     "Like `cl--simple-expr-p' but for a list of expressions."
>
>>  (defun cl--const-expr-p (x)
>> +  "Check if X is constant (i.e., no side effects or dependencies).
>> +
>> +See `macroexp-const-p' for similar functionality without cl-lib dependency."
>
> "Similar" fails to describe the difference :-(
> [ Note, I can't blame you: I know the difference but I couldn't come up
>   with a useful description of it, that is, one that is easier to
>   understand than the code itself.  FWIW I believe the difference is
>   a latent bug, which is partly why I'd like to delete the function :-)  ]
>
>
>         Stefan

Message #22 received at 66863-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Jeremy Bryant <jb <at> jeremybryant.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 66863-done <at> debbugs.gnu.org
Subject: Re: bug#66863: [PATCH] Add two docstrings in cl-macs.el
Date: Sat, 4 Nov 2023 03:41:56 -0700

Jeremy Bryant via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> gnu.org> writes:

> Thank you, I've made a note on conventions.  For this patch, as you have
> pushed, does this mean this is closed?

I think so, yes.  Now done.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.