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

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

[Jeremy Bryant via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 280 bytes --]

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 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Add-two-docstrings-in-cl-macs.el.patch --]
[-- Type: text/patch, Size: 1118 bytes --]

From 9a0d8649511377a160f410fd73f74558f4a8e9dc Mon Sep 17 00:00:00 2001
From: Jeremy Bryant <jb@jeremybryant.net>
Date: Tue, 31 Oct 2023 22:24:20 +0000
Subject: [PATCH] Add two docstrings in cl-macs.el

* lisp/emacs-lisp/cl-macs.el (cl--simple-exprs-p)
(cl--const-expr-p): Add docstrings.
---
 lisp/emacs-lisp/cl-macs.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 43207ce7026..fd19bca60d9 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -101,6 +101,7 @@ cl--simple-expr-p
     (and (> size 0) (1- size))))
 
 (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))
@@ -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))
-- 
2.40.1

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> * 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

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

[Jeremy Bryant via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: 0001-Add-two-docstrings-in-cl-macs.el.patch --]
[-- Type: text/x-diff, Size: 1274 bytes --]

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

* lisp/emacs-lisp/cl-macs.el (cl--simple-exprs-p)
(cl--const-expr-p): Add docstrings
---
 lisp/emacs-lisp/cl-macs.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/cl-macs.el b/lisp/emacs-lisp/cl-macs.el
index 43207ce7026..15b56d0e322 100644
--- a/lisp/emacs-lisp/cl-macs.el
+++ b/lisp/emacs-lisp/cl-macs.el
@@ -101,6 +101,7 @@ cl--simple-expr-p
     (and (> size 0) (1- size))))
 
 (defun cl--simple-exprs-p (xs)
+  "Map `cl--simple-expr-p' to each element of list XS."
   (while (and xs (cl--simple-expr-p (car xs)))
     (setq xs (cdr xs)))
   (not xs))
@@ -116,8 +117,10 @@ cl--safe-expr-p
 	     (while (and (setq x (cdr x)) (cl--safe-expr-p (car x))))
 	     (null x)))))
 
-;;; 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).
+
+See `macroexp-const-p' for similar functionality without cl-lib dependency."
   (cond ((consp x)
 	 (or (eq (car x) 'quote)
 	     (and (memq (car x) '(function cl-function))
-- 
2.40.1


[-- Attachment #2: Type: text/plain, Size: 1509 bytes --]


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

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

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> From 16298965905b1859aa8a342b98803f4abd6da3f0 Mon Sep 17 00:00:00 2001
> From: Jeremy Bryant <jb@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

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

[Jeremy Bryant via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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@iro.umontreal.ca> writes:

>> From 16298965905b1859aa8a342b98803f4abd6da3f0 Mon Sep 17 00:00:00 2001
>> From: Jeremy Bryant <jb@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

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

[Stefan Kangas]

Jeremy Bryant via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@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.