GNU bug report logs - #72334
[PATCH] Always print commas and comma-ats specially

Previous Next

To reply to this bug, email your comments to 72334 AT debbugs.gnu.org.

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

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

From: Thuna <thuna.cing <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Always print commas and comma-ats specially
Date: Sun, 28 Jul 2024 15:30:40 +0200

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

In 55481 the recursion was fixed but backquotes and comma(-at)s were
left as-is, so as to print comma(-at)s specially only if they are
escaping a backquote.

This is not particularly useful (and I would argue is bad), so something
like the attached patch could be a good starting point to make it so
that they are always printed specially (given `print-quoted' is non-nil
and it is a proper list of length two - the usual checks for other
quoted forms), which also solves(maybe? - it was not the primary goal)
the recursion.  Feel free to change or drop whatever.

[0001-Always-print-commas-and-comma-ats-specially-with-pri.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 11 Feb 2025 11:23:19 -0800

Thuna <thuna.cing <at> gmail.com> writes:

> In 55481 the recursion was fixed but backquotes and comma(-at)s were
> left as-is, so as to print comma(-at)s specially only if they are
> escaping a backquote.
>
> This is not particularly useful (and I would argue is bad), so something
> like the attached patch could be a good starting point to make it so
> that they are always printed specially (given `print-quoted' is non-nil
> and it is a proper list of length two - the usual checks for other
> quoted forms), which also solves(maybe? - it was not the primary goal)
> the recursion.  Feel free to change or drop whatever.

Could you please summarize the issue that this is intended to fix?

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

From: Thuna <thuna.cing <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 11 Feb 2025 20:35:03 +0100

> Could you please summarize the issue that this is intended to fix?

(cons 'foo (list '\, 'bar)) should be printed as (foo . ,bar) instead of
(foo \, bar).  Same for (foo . ,@bar) and (foo . `bar), and maybe even
(foo . 'bar) and (foo . #'bar).

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 11 Feb 2025 12:03:34 -0800

Thuna <thuna.cing <at> gmail.com> writes:

>> Could you please summarize the issue that this is intended to fix?
>
> (cons 'foo (list '\, 'bar)) should be printed as (foo . ,bar) instead of
> (foo \, bar).  Same for (foo . ,@bar) and (foo . `bar), and maybe even
> (foo . 'bar) and (foo . #'bar).

Stefan M, any comments?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org, Thuna <thuna.cing <at> gmail.com>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 11 Feb 2025 15:59:06 -0500

>>> Could you please summarize the issue that this is intended to fix?
>> (cons 'foo (list '\, 'bar)) should be printed as (foo . ,bar) instead of
>> (foo \, bar).  Same for (foo . ,@bar) and (foo . `bar), and maybe even
>> (foo . 'bar) and (foo . #'bar).
> Stefan M, any comments?

I haven't looked at the patch, but the goes above sounds good.
It doesn't occur very often in my experience, but when it occurs it does
look odd and can easily confuse the reader.


        Stefan

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 72334 <at> debbugs.gnu.org, Thuna <thuna.cing <at> gmail.com>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 11 Feb 2025 15:37:44 -0800

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

>>>> Could you please summarize the issue that this is intended to fix?
>>> (cons 'foo (list '\, 'bar)) should be printed as (foo . ,bar) instead of
>>> (foo \, bar).  Same for (foo . ,@bar) and (foo . `bar), and maybe even
>>> (foo . 'bar) and (foo . #'bar).
>> Stefan M, any comments?
>
> I haven't looked at the patch, but the goes above sounds good.
> It doesn't occur very often in my experience, but when it occurs it does
> look odd and can easily confuse the reader.

Thanks.

Thuna, I didn't study your patch very deeply yet but the first thing
that stands out is that there are no tests.  Any chance you could look
into that?

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

From: Thuna <thuna.cing <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, Stefan Monnier
 <monnier <at> iro.umontreal.ca>
Cc: 72334 <at> debbugs.gnu.org
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Wed, 12 Feb 2025 01:06:45 +0100

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

> Thanks.
>
> Thuna, I didn't study your patch very deeply yet but the first thing
> that stands out is that there are no tests.  Any chance you could look
> into that?

Sure.  It's somewhat late here so I'll write those tomorrow.  What
should quote and function do?  That is, should it be (foo . 'bar) or
(foo quote bar)?

The important thing is, though, I actually took a look at the patch
since it's been a while, and I was wrong, what I explained was bug
72434, which I've also attached here.

This patch _actually_ makes it so that (\, foo) [and (\,@ foo)] is
printed as ",foo" in every context, whereas it current prints like that
only if we are inside a quasiquoted form [that is, (\` (\, (\, foo))
prints as "`,(\, foo)"].

[If you're trying the other patch, I should note here as well that that
one builds on top of this one, so the results are incorrect if you only
apply that.  Specifically, when we're not in a backquote, (foo . ,bar)
prints as (foo . (\, bar)) instead.]

[0001-Print-list-terminating-backquote-and-comma-forms-spe.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Thuna <thuna.cing <at> gmail.com>, Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 72334 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 11 Feb 2025 18:57:25 -0800

Thuna <thuna.cing <at> gmail.com> writes:

>> Thuna, I didn't study your patch very deeply yet but the first thing
>> that stands out is that there are no tests.  Any chance you could look
>> into that?
>
> Sure.  It's somewhat late here so I'll write those tomorrow.

Thanks!

> What should quote and function do?  That is, should it be (foo . 'bar)
> or (foo quote bar)?

I don't have a strong opinion on that.  A priori, I think what we have
now is fine, but let's see what other people think.

BTW, I note that while in ELisp

    ,bar

prints as

    (\, bar)

in Scheme it prints as the somewhat nicer

    (unquote bar)

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

From: Andreas Schwab <schwab <at> suse.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Thuna <thuna.cing <at> gmail.com>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Wed, 12 Feb 2025 09:32:19 +0100

On Feb 11 2025, Stefan Kangas wrote:

> BTW, I note that while in ELisp
>
>     ,bar
>
> prints as
>
>     (\, bar)

(car ',bar) => \,

> in Scheme it prints as the somewhat nicer
>
>     (unquote bar)

(car ',bar) => unquote

They just use different symbols to implement the \, reader macro.

-- 
Andreas Schwab, SUSE Labs, schwab <at> suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Andreas Schwab <schwab <at> suse.de>
Cc: 72334 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>, Thuna <thuna.cing <at> gmail.com>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Wed, 12 Feb 2025 00:53:39 -0800

Andreas Schwab <schwab <at> suse.de> writes:

> On Feb 11 2025, Stefan Kangas wrote:
>
>> BTW, I note that while in ELisp
>>
>>     ,bar
>>
>> prints as
>>
>>     (\, bar)
>
> (car ',bar) => \,
>
>> in Scheme it prints as the somewhat nicer
>>
>>     (unquote bar)
>
> (car ',bar) => unquote
>
> They just use different symbols to implement the \, reader macro.

Yes, of course.  I'm saying that ours is.. less pretty.

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

From: Thuna <thuna.cing <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>, Stefan Monnier
 <monnier <at> iro.umontreal.ca>
Cc: 72334 <at> debbugs.gnu.org
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Wed, 12 Feb 2025 23:55:46 +0100

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

Ok, I have the tests.  In the process I found what I think are bugs in
print-tests.el and realized that I need to patch cl-print.el as well, so
I have all of those here as well, as separate patches.

[0001-Fix-print-tests.patch (text/x-patch, attachment)]

[0002-Add-tests-for-printing-comma-and-backquote-at-the-ta.patch (text/x-patch, attachment)]

[0003-Print-commas-and-backquotes-correctly.patch (text/x-patch, attachment)]

[0004-Print-backquotes-and-commas-correctly-in-cl-print.el.patch (text/x-patch, attachment)]

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

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Thuna <thuna.cing <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Sun, 23 Feb 2025 00:48:45 +0000

Thuna <thuna.cing <at> gmail.com> writes:

> Ok, I have the tests.  In the process I found what I think are bugs in
> print-tests.el and realized that I need to patch cl-print.el as well, so
> I have all of those here as well, as separate patches.

Thanks, I have some comments on the last two patches.

Meanwhile, your first patch LGTM, so I installed that one to get it out
of the way.

> From 8b4d1ff7ef49a7f0b6e432cf03b7aa33c7fd7e48 Mon Sep 17 00:00:00 2001
> From: Thuna <thuna.cing <at> gmail.com>
> Date: Wed, 12 Feb 2025 23:40:44 +0100
> Subject: [PATCH 3/4] Print commas and backquotes correctly
>
> * src/print.c (print_object): Print commas and comma-ats as special
> regardless of whether they are in a backquote or not, so long as
> print-quoted is non-nil.  Print tailing backquote, comma, and comma-at
> forms as atoms and not lists.  Calculate the head of OBJ once.

Missing a bug number here:   (Bug#72334)

> ---
>  src/print.c | 58 ++++++++++++++++++++++-------------------------------
>  1 file changed, 24 insertions(+), 34 deletions(-)
>
> diff --git a/src/print.c b/src/print.c
> index c7cba5bface..adc63ec253c 100644
> --- a/src/print.c
> +++ b/src/print.c
> @@ -2483,48 +2483,32 @@ print_object (Lisp_Object obj, Lisp_Object printcharfun, bool escapeflag)
>        break;
>
>      case Lisp_Cons:
> +      Lisp_Object car = XCAR (obj);

I guess you do this here because of caching, but now we don't know that

    CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj))

so don't we risk a crash here?

>        /* If deeper than spec'd depth, print placeholder.  */
>        if (FIXNUMP (Vprint_level)
>  	  && print_depth > XFIXNUM (Vprint_level))
>  	print_c_string ("...", printcharfun);
>        else if (print_quoted && CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj)))
> -	       && EQ (XCAR (obj), Qquote))
> +	       && (EQ (car, Qquote) ||
> +		   EQ (car, Qfunction) ||
> +		   EQ (car, Qbackquote) ||
> +		   EQ (car, Qcomma) ||
> +		   EQ (car, Qcomma_at)))
>  	{
> -	  printchar ('\'', printcharfun);
> +	  if (EQ (car, Qquote))
> +	    printchar ('\'', printcharfun);
> +	  else if (EQ (car, Qfunction))
> +	    print_c_string("#'", printcharfun);
> +          else if (EQ (car, Qbackquote))
> +	    printchar ('`', printcharfun);
> +          else if (EQ (car, Qcomma))
> +	    printchar (',', printcharfun);
> +          else if (EQ (car, Qcomma_at))
> +	    print_c_string (",@", printcharfun);

Hmm, I don't know if it matters except for code cleanliness, but I'm
wondering if we can avoid testing for the same thing twice.

Can't you make the same additions in the code below instead?  Or
reorganize it somehow to be more clean?

>  	  obj = XCAR (XCDR (obj));
>  	  --print_depth;	/* tail recursion */
>  	  goto print_obj;
>  	}
> -      else if (print_quoted && CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj)))
> -	       && EQ (XCAR (obj), Qfunction))
> -	{
> -	  print_c_string ("#'", printcharfun);
> -	  obj = XCAR (XCDR (obj));
> -	  --print_depth;	/* tail recursion */
> -	  goto print_obj;
> -	}
> -      /* FIXME: Do we really need the new_backquote_output gating of
> -	 special syntax for comma and comma-at?  There is basically no
> -	 benefit from it at all, and it would be nice to get rid of
> -	 the recursion here without additional complexity.  */
> -      else if (print_quoted && CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj)))
> -	       && EQ (XCAR (obj), Qbackquote))
> -	{
> -	  printchar ('`', printcharfun);
> -	  new_backquote_output++;
> -	  print_object (XCAR (XCDR (obj)), printcharfun, escapeflag);
> -	  new_backquote_output--;
> -	}
> -      else if (print_quoted && CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj)))
> -	       && (EQ (XCAR (obj), Qcomma)
> -		   || EQ (XCAR (obj), Qcomma_at))
> -	       && new_backquote_output)
> -	{
> -	  print_object (XCAR (obj), printcharfun, false);
> -	  new_backquote_output--;
> -	  print_object (XCAR (XCDR (obj)), printcharfun, escapeflag);
> -	  new_backquote_output++;
> -	}
>        else
>  	{
>  	  printchar ('(', printcharfun);
> @@ -2547,7 +2531,7 @@ print_object (Lisp_Object obj, Lisp_Object printcharfun, bool escapeflag)
>  		  .u.list.tortoise_idx = 0,
>  		});
>  	      /* print the car */
> -	      obj = XCAR (obj);
> +	      obj = car;
>  	      goto print_obj;
>  	    }
>  	}
> @@ -2670,7 +2654,13 @@ print_object (Lisp_Object obj, Lisp_Object printcharfun, bool escapeflag)
>  		--print_depth;
>  		goto next_obj;
>  	      }
> -	    else if (CONSP (next))
> +	    else if (CONSP (next)
> +		     && !(print_quoted
> +			  && (EQ (Qbackquote, XCAR (next))
> +			      || EQ (Qcomma, XCAR (next))
> +			      || EQ (Qcomma_at, XCAR (next)))
> +			  && CONSP (XCDR (next))
> +			  && NILP (XCDR (XCDR (next)))))
>  	      {
>  		if (!NILP (Vprint_circle))
>  		  {
> --
> 2.44.2
>
>
> From c6a9843680ebb075ff0d0041ca590656e06bcee7 Mon Sep 17 00:00:00 2001
> From: Thuna <thuna.cing <at> gmail.com>
> Date: Wed, 12 Feb 2025 23:42:48 +0100
> Subject: [PATCH 4/4] Print backquotes and commas correctly in cl-print.el
>
> * lisp/emacs-lisp/cl-print.el (cl-print--cons-tail): Delete function.
> (cl-print-object): Refactor to do the work of `cl-print--cons-tail' as
> well.  Print tailing backquote, comma, and comma-ats as atoms.

Missing bug number here also:   (Bug#72334)

> ---
>  lisp/emacs-lisp/cl-print.el | 75 ++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 34 deletions(-)
>
> diff --git a/lisp/emacs-lisp/cl-print.el b/lisp/emacs-lisp/cl-print.el
> index 5af34361b92..4ff8c58eae1 100644
> --- a/lisp/emacs-lisp/cl-print.el
> +++ b/lisp/emacs-lisp/cl-print.el
> @@ -66,43 +66,50 @@ cl-print-object-contents
>    (error "Missing cl-print-object-contents method"))
>
>  (cl-defmethod cl-print-object ((object cons) stream)
> -  (if (and cl-print--depth (natnump print-level)
> -           (> cl-print--depth print-level))
> -      (cl-print-insert-ellipsis object nil stream)
> -    (let ((car (pop object)))
> -      (if (and print-quoted
> -               (memq car '(\, quote function \` \,@ \,.))
> -               (consp object)
> -               (null (cdr object)))
> -          (progn
> -            (princ (cond
> -                    ((eq car 'quote) '\')
> -                    ((eq car 'function) "#'")
> -                    (t car))
> -                   stream)
> -            (cl-print-object (car object) stream))
> -        (princ "(" stream)
> -        (cl-print--cons-tail car object stream)
> -        (princ ")" stream)))))
> -
> -(defun cl-print--cons-tail (car object stream)
> -  (let ((count 1))
> -    (cl-print-object car stream)
> -    (while (and (consp object)
> -                (not (cond
> +  (let ((count 0))
> +    (cl-flet* ((quotedp (obj &optional tailp)
> +		 (and print-quoted
> +                      (consp obj)
> +                      (consp (cdr obj))
> +                      (null (cddr obj))
> +                      (or (and (not tailp) (memq (car object) '(function quote)))
> +                          (memq (car obj) '(\` \, \,@ \,.)))))
> +               (endp (tail)
> +		 (or (atom tail)
> +                     (quotedp tail t)
> +		     (cond
> +		      ((zerop count) nil)
>                        (cl-print--number-table
>                         (numberp (gethash object cl-print--number-table)))
>                        ((memq object cl-print--currently-printing))
> -                      (t (push object cl-print--currently-printing)
> -                         nil))))
> -      (princ " " stream)
> -      (if (or (not (natnump print-length)) (> print-length count))
> -          (cl-print-object (pop object) stream)
> -        (cl-print-insert-ellipsis object t stream)
> -        (setq object nil))
> -      (cl-incf count))
> -    (when object
> -      (princ " . " stream) (cl-print-object object stream))))
> +		      (t (push object cl-print--currently-printing)
> +			 nil)))))
> +      (cond
> +       ((and cl-print--depth (natnump print-level)
> +             (> cl-print--depth print-level))
> +	(cl-print-insert-ellipsis object nil stream))
> +       ((quotedp object)
> +	(princ (cl-case (car object)
> +		 (quote "'")
> +		 (function "#'")
> +		 (t (car object)))
> +               stream)
> +	(cl-print-object (cadr object) stream))
> +       (t
> +	(princ "(" stream)
> +	(while (not (endp object))
> +          (if (and (natnump print-length) (>= count print-length))
> +	      (progn
> +                (cl-print-insert-ellipsis object t stream)
> +                (setq object nil))
> +            (cl-print-object (pop object) stream)
> +            (when object
> +	      (princ " " stream)))
> +          (cl-incf count))
> +	(when object
> +          (princ ". " stream)
> +          (cl-print-object object stream))
> +	(princ ")" stream))))))

Hmm, this is a little bit hard to review.  Do we really need to inline
`cl-print--cons-tail`?  Just as a very high-level comment, it seems like
we go from two relatively small functions to one big and complex one.
Is that justified?

>
>  (cl-defmethod cl-print-object-contents ((object cons) _start stream)
>    (cl-print--cons-tail (car object) (cdr object) stream))

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

From: Thuna <thuna.cing <at> gmail.com>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 72334 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#72334: [PATCH] Always print commas and comma-ats specially
Date: Tue, 04 Mar 2025 22:02:55 +0100

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

>> ---
>>  src/print.c | 58 ++++++++++++++++++++++-------------------------------
>>  1 file changed, 24 insertions(+), 34 deletions(-)
>>
>> diff --git a/src/print.c b/src/print.c
>> index c7cba5bface..adc63ec253c 100644
>> --- a/src/print.c
>> +++ b/src/print.c
>> @@ -2483,48 +2483,32 @@ print_object (Lisp_Object obj, Lisp_Object printcharfun, bool escapeflag)
>>        break;
>>
>>      case Lisp_Cons:
>> +      Lisp_Object car = XCAR (obj);
>
> I guess you do this here because of caching, but now we don't know that
>
>     CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj))
>
> so don't we risk a crash here?

That's being checked for in
>>        else if (print_quoted && CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj)))
no?  Or am I misunderstanding the problem?

>>        /* If deeper than spec'd depth, print placeholder.  */
>>        if (FIXNUMP (Vprint_level)
>>  	  && print_depth > XFIXNUM (Vprint_level))
>>  	print_c_string ("...", printcharfun);
>>        else if (print_quoted && CONSP (XCDR (obj)) && NILP (XCDR (XCDR (obj)))
>> -	       && EQ (XCAR (obj), Qquote))
>> +	       && (EQ (car, Qquote) ||
>> +		   EQ (car, Qfunction) ||
>> +		   EQ (car, Qbackquote) ||
>> +		   EQ (car, Qcomma) ||
>> +		   EQ (car, Qcomma_at)))
>>  	{
>> -	  printchar ('\'', printcharfun);
>> +	  if (EQ (car, Qquote))
>> +	    printchar ('\'', printcharfun);
>> +	  else if (EQ (car, Qfunction))
>> +	    print_c_string("#'", printcharfun);
>> +          else if (EQ (car, Qbackquote))
>> +	    printchar ('`', printcharfun);
>> +          else if (EQ (car, Qcomma))
>> +	    printchar (',', printcharfun);
>> +          else if (EQ (car, Qcomma_at))
>> +	    print_c_string (",@", printcharfun);
>
> Hmm, I don't know if it matters except for code cleanliness, but I'm
> wondering if we can avoid testing for the same thing twice.
>
> Can't you make the same additions in the code below instead?  Or
> reorganize it somehow to be more clean?

I don't really see a straightforward way to do that while not repeating
the `else' section below.  You could have a `type = X' in the condition
to avoid having to compare `car' again, but that would be about it.

> Hmm, this is a little bit hard to review.  Do we really need to inline
> `cl-print--cons-tail`?  Just as a very high-level comment, it seems like
> we go from two relatively small functions to one big and complex one.
> Is that justified?

Hm, yes, I can see how that would be difficult to review.  This can (I
think; I've not tested) be done with a small patch, which I've attached,
but I think that it's overall easier to reason about the large function,
which I've outlined below.  Nevertheless, feel free to go with either
(though do run the tests on the attached patch, please).

> (cl-defmethod cl-print-object ((object cons) stream)
>   (let ((count 0))
>     (cl-flet* (
The local function `quotedp' checks if OBJ should be quoted (that is,
"'foo", "#'foo", etc.).  If TAILP is non-nil, then OBJ should be quoted
even when it's at a tail position (the difference between "(foo . ,bar)"
and "(foo quote bar)").
>                (quotedp (obj &optional tailp)
>                  (and print-quoted
>                       (consp obj)
>                       (consp (cdr obj))
>                       (null (cddr obj))
>                       (or (and (not tailp) (memq (car object) '(function quote)))
>                           (memq (car obj) '(\` \, \,@ \,.)))))
The local function `endp' checks if TAIL terminates the printing of a
list at the cdr position (that is, an atom, a [tail] quoted form, a
backref [#N#], or a circular object [#N=]).
>                (endp (tail)
>                  (or (atom tail)
>                      (quotedp tail t)
>                      (cond
>                       ((zerop count) nil)
>                       (cl-print--number-table
>                        (numberp (gethash object cl-print--number-table)))
>                       ((memq object cl-print--currently-printing))
>                       (t (push object cl-print--currently-printing)
>                          nil)))))
>       (cond
This branch is when we exceed the depth limit.
>        ((and cl-print--depth (natnump print-level)
>              (> cl-print--depth print-level))
>         (cl-print-insert-ellipsis object nil stream))
This branch is when OBJECT is quoted.
>        ((quotedp object)
>         (princ (cl-case (car object)
>                  (quote "'")
>                  (function "#'")
>                  (t (car object)))
>                stream)
>         (cl-print-object (cadr object) stream))
This branch is when OBJECT is a non-quoted cons.  My understanding is
that backrefs and circular objects would have already been handled by
:around/:before methods, so we can immediately assume we're dealing with
a normal list.
>        (t
>         (princ "(" stream)
We go until the current object terminates the normal printing of a list.
Since OBJECT is not circular or a backref (by our previous assumption),
quoted (due to the previous branch), or a non-cons (by the method
specialization), we will not hit this immediately, so we will at least
print one item (which is a necessary condition, given we already opened
a parenthesis).
>         (while (not (endp object))
If we exceed the length, we print an ellipsis and exit such that we
immediately insert a closing parenthesis.
>           (if (and (natnump print-length) (>= count print-length))
>               (progn
>                 (cl-print-insert-ellipsis object t stream)
>                 (setq object nil))
Otherwise we print the current item and move along.
>             (cl-print-object (pop object) stream)
Unless the new tail is nil, we will be printing _something_, whether it
be another item or a dot, so we can directly print a space to avoid
having to do it later.
>             (when object
>               (princ " " stream)))
>           (cl-incf count))
When we're here, `(endp object)' will be true.  If OBJECT is nil, then
we just insert a closing parenthesis, otherwise we insert ". "
(remember, we already inserted the space in the `while' loop) and then
print the final tail before.
>         (when object
>           (princ ". " stream)
Since OBJECT here is either an atom, a quoted form, a backref, or a
circular object, calling cl-print-object will always do the right thing.
The only non-obvious one is the quoted form - that will call this method
again, going into the `(quotedp object)' branch [It might get stuck in
the depth branch?  I am not fully sure about that.] and print normally.
>           (cl-print-object object stream))
>         (princ ")" stream))))))

[0004b-Print-backquotes-and-commas-correctly-in-cl-print.el.patch (text/x-patch, attachment)]

Previous Next

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