GNU bug report logs - #63509
[PATCH] Make copy-tree work with records

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 63509 in the body.
You can then email your comments to 63509 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: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Make copy-tree work with records
Date: Sun, 14 May 2023 20:57:17 -0700

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

Hello,

copy-tree does not currently work with records:

(cl-defstruct foo bar)
(let* ((rec (make-foo :bar "hello"))
       (copy (copy-tree rec t)))
  (setf (foo-bar copy) "goodbye")
  (foo-bar rec))

Expected "hello"; actual "goodbye".

Attached patch fixes this behavior.

Please tell me if I misunderstand the intended behavior of copy-tree.

Thank you,

Joseph

[0001-Make-copy-tree-work-with-records.patch (text/x-diff, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 63509 <at> debbugs.gnu.org
Subject: Re: bug#63509: [PATCH] Make copy-tree work with records
Date: Mon, 15 May 2023 14:26:31 +0300

> Date: Sun, 14 May 2023 20:57:17 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> copy-tree does not currently work with records:

It isn't documented to work with anything but cons cells or vectors.

> Attached patch fixes this behavior.

Thanks, but such a change must come with:

 - NEWS entry
 - update for the ELisp manual
 - preferably added tests to regression-test this feature

I also think we should rename the second argument, as VECP no longer
fits.

> Please tell me if I misunderstand the intended behavior of copy-tree.

Adding Stefan in case he has comments.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63509 <at> debbugs.gnu.org, Joseph Turner <joseph <at> breatheoutbreathe.in>
Subject: Re: bug#63509: [PATCH] Make copy-tree work with records
Date: Mon, 15 May 2023 10:42:24 -0400

>> Please tell me if I misunderstand the intended behavior of copy-tree.
> Adding Stefan in case he has comments.

No specific comment, no.  I don't hold `copy-tree` in my heart (it
rarely copies exactly what you need, similar problem as for
`flatten-tree`), so as general rule I'd recommend against its use, but
it's occasionally handy as a quick&dirty "deep copy" function, and
extending it to records seem like an obvious fix (it used to do that
back when `cl-defstruct` used vectors).


        Stefan

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63509 <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#63509: [PATCH] Make copy-tree work with records
Date: Mon, 15 May 2023 10:59:57 -0700

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

Eli Zaretskii <eliz <at> gnu.org> writes:

>  - NEWS entry

I updated the 29 entry. Should I move it to 30?

>  - update for the ELisp manual

Changes made in patch:

- updated the entry in lists.texi
- added a new entry in records.texi
- added entry in the vector group in shortdoc.el

>  - preferably added tests to regression-test this feature

Done.

> I also think we should rename the second argument, as VECP no longer
> fits.

How about VECTOR-LIKE-P? See patch.

Joseph

[0001-Make-copy-tree-work-with-records.patch (text/x-diff, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63509 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#63509: [PATCH] Make copy-tree work with records
Date: Thu, 18 May 2023 13:53:59 +0300

> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 63509 <at> debbugs.gnu.org
> Date: Mon, 15 May 2023 10:59:57 -0700
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >  - NEWS entry
> 
> I updated the 29 entry. Should I move it to 30?

Yes, this new feature will be installed on the master branch, which
will become Emacs 30.

> --- a/doc/lispref/records.texi
> +++ b/doc/lispref/records.texi
> @@ -81,6 +81,18 @@ This function returns a new record with type @var{type} and
>  @end example
>  @end defun
>  
> +@defun copy-tree tree &optional vector-like-p
> +This function copies a record when @var{vector-like-p} is
> +non-@code{nil}.
> +
> +@example
> +@group
> +(copy-tree (record 'foo "a"))
> +     @result{} #s(foo "a")
> +@end group
> +@end example
> +@end defun

This addition is redundant.  We don't describe the same function in
more than one place.  If there are reasons to mention it in other
places, we just add there a short note with a cross-reference to the
detailed description.

> ++++
> +** 'copy-tree' now correctly copies records when its optional second

The "correctly" part hints that the previous behavior was a bug, which
it wasn't (and we don't mention bugfixes in NEWS anyway).  So I would
rephrase

  'copy-tree' can now copy records as well, when its optional...

> +argument is non-nil.  The second argument has been renamed from VECP
> +to VECTOR-LIKE-P since it now works with both vectors and records.

The last sentence should be removed: we don't mention such minor
details in NEWS, unless the change is an incompatible change.

Last, but not least: please always accompany your changes with
ChageLog-style commit log messages describing the changes.  You can
find more information about this in the file CONTRIBUTE in the Emacs
tree, and you can see many examples by typing "git log" in the
repository.

Thanks.

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

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 63509 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#63509: [PATCH] Make copy-tree work with records
Date: Thu, 18 May 2023 12:05:57 -0700

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

Eli Zaretskii <eliz <at> gnu.org> writes:

> Yes, this new feature will be installed on the master branch, which
> will become Emacs 30.

Moved note from NEWS.29 to NEWS.

>> --- a/doc/lispref/records.texi
>> +++ b/doc/lispref/records.texi
>> @@ -81,6 +81,18 @@ This function returns a new record with type @var{type} and
>>  @end example
>>  @end defun
>>
>> +@defun copy-tree tree &optional vector-like-p
>> +This function copies a record when @var{vector-like-p} is
>> +non-@code{nil}.
>> +
>> +@example
>> +@group
>> +(copy-tree (record 'foo "a"))
>> +     @result{} #s(foo "a")
>> +@end group
>> +@end example
>> +@end defun
>
> This addition is redundant.  We don't describe the same function in
> more than one place.  If there are reasons to mention it in other
> places, we just add there a short note with a cross-reference to the
> detailed description.

Replaced @defun with a short sentence with @pxref.

>> ++++
>> +** 'copy-tree' now correctly copies records when its optional second
>
> The "correctly" part hints that the previous behavior was a bug, which
> it wasn't (and we don't mention bugfixes in NEWS anyway).  So I would
> rephrase
>
>   'copy-tree' can now copy records as well, when its optional...
>
>> +argument is non-nil.  The second argument has been renamed from VECP
>> +to VECTOR-LIKE-P since it now works with both vectors and records.
>
> The last sentence should be removed: we don't mention such minor
> details in NEWS, unless the change is an incompatible change.

Done.

> Last, but not least: please always accompany your changes with
> ChageLog-style commit log messages describing the changes.  You can
> find more information about this in the file CONTRIBUTE in the Emacs
> tree, and you can see many examples by typing "git log" in the
> repository.

Done.

Please let me know if any further changes need to be made!

Best,

Joseph

[0001-Make-copy-tree-work-with-records.patch (text/x-diff, attachment)]

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: 63509-done <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#63509: [PATCH] Make copy-tree work with records
Date: Fri, 19 May 2023 09:07:10 +0300

> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Cc: monnier <at> iro.umontreal.ca, 63509 <at> debbugs.gnu.org
> Date: Thu, 18 May 2023 12:05:57 -0700
> 
> Please let me know if any further changes need to be made!

Thanks, installed on master, and closing the bug.

Previous Next

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