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" <bug-gnu-emacs@gnu.org>]

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