Add copy-tree

[josephmturner]

Resolves #24.

I copied the tests from /test/lisp/subr-tests.el. Would it be appropriate to indicate that the tests were copied from Emacs? Perhaps you'd prefer shorter tests?

[minad]

On 5/22/23 00:06, Joseph Turner wrote: Resolves #24 <#24>.

Thanks!

I copied the tests from |/test/lisp/subr-tests.el|. Would it be appropriate to indicate that the tests were copied from Emacs?

Yes, a short comment is appropriate. However many tests are copied from Emacs. At some point we should think about possibly synchronizing the test suites. The CI is currently failing on old Emacs versions due to invalid literals. Records have been introduced in 26.1. The test suite should make sure that it tests the degraded functionality on Emacs < 26.1 gracefully.

[josephmturner]

Yes, a short comment is appropriate.

Done.

However many tests are copied from Emacs. At some point we should think about possibly synchronizing the test suites.

This sounds like a good idea! Although in this case, it wouldn't work...

[josephmturner]

I see now that I forgot to change the final literal. Fixed in the latest force-push.

But... IIUC the printed representation of structs are different pre- and post-26, so I'm not sure how to do this without a conditional.

I think we have to use make-record here instead, such that the reader of Emacs < 26.1 accepts the syntax. Then conditionally enable the record tests only on Emacs >= 26.1.

If the latest force-push doesn't work, could we keep the existing test which uses cl-defstruct to appease the pre-26 reader, and then wrap everything after the first let-block with (when (version<= "26.1" emacs-version) ...)?

According to Stefan , "[copy-tree] used to [work with structs]
back when cl-defstruct used vectors", so for pre-26 users, compat--copy-tree isn't necessary for dealing with structs anyway.

[minad]

But... IIUC the printed representation of structs are different pre- and post-26, so I'm not sure how to do this without a conditional.

Yes, using the printed representation for the test is not ideal. You could still use a condition (if (< emacs-major-version 26) str1 str2), where str1 is the old string representation.

According to Stefan , "[copy-tree] used to [work with structs]
back when cl-defstruct used vectors", so for pre-26 users, compat--copy-tree isn't necessary for dealing with structs anyway.

That's right. But our test suite should cover all the supported Emacs versions in any case.

[josephmturner]

Yes, using the printed representation for the test is not ideal. You could still use a condition (if (< emacs-major-version 26) str1 str2), where str1 is the old string representation.

Okay! What would the string representation be for pre 26 versions?

[minad]

Okay! What would the string representation be for pre 26 versions?

Run Emacs 25 :)

[minad]

You could just guess the representation by looking at the old cl-defstruct definition. I suspect it was just the vector representation, e.g., [foo ...] for a foo struct, but I haven't checked.

[josephmturner]

You could just guess the representation by looking at the old cl-defstruct definition. I suspect it was just the vector representation, e.g., [foo ...] for a foo struct, but I haven't checked.

Thanks, I'll do that. I was not looking forward to building Emacs 25.

[phikal]

Joseph Turner ***@***.***> writes:

> You could just guess the representation by looking at the old `cl-defstruct` definition. I suspect it was just the vector representation, e.g., `[foo ...]` for a `foo` struct, but I haven't checked. Thanks, I'll do that. I was not looking forward to building Emacs 25.

In case you use Guix, you can build older versions using these package definitions: https://git.sr.ht/~pkal/guix-emacs-historical If not, I can build it for you and send you a portable tarball.

[josephmturner]

https://git.sr.ht/~pkal/guix-emacs-historical

Thank you!! This looks like a useful resource.

The pre-26 representation is [cl-struct-foo value] (I downloaded the 25 source, then renamed and evaled the old cl-defstruct definition)

[minad]

@phikal Could you make these portable tarballs available somewhere, ideally signed?

[phikal]

Daniel Mendler ***@***.***> writes:

@phikal Could you make these portable tarballs available somewhere, ideally signed?

I could try it, but 1. I've never done that before, 2. I don't have the storage to host these files myself (the storage is not insignificant, since it contains Emacs and all the dependencies).

[josephmturner]

It looks like the problem now is that the pre-26 versions are failing due to (void-function recordp):

https://github.com/emacs-compat/compat/actions/runs/5043804459/jobs/9046066759?pr=25#step:6:66

Since recordp is defined in the C code, should we just mark compat--copy-tree as only available for 26 and later?

[phikal]

Since recordp is defined in the C code, should we just mark compat--copy-tree as only available for 26 and later?

No, I would oppose that.

We can either define a compatibility predicate recordp that would always return nil (since records were added in 26, though I don't think this would work), or we can check if recordp is fboundp before invoking it.

[josephmturner]

We can either define a compatibility predicate recordp that would always return nil (since records were added in 26, though I don't think this would work), or we can check if recordp is fboundp before invoking it.

Okay, will do.

[minad]

I agree with Philip.

Also we should not add recordp support to Compat, since we cannot reliably port back the record functionality. Please just check (fboundp 'recordp).

[phikal]

Yes, a short comment is appropriate. However many tests are copied from Emacs. At some point we should think about possibly synchronizing the test suites.

A number of our tests are more extensive than what Emacs has, which is the reason why the core has expressed interest in copying our code. So there is certainly potential for collaboration here.

[josephmturner]

I have wrapped the body of compat--copy-tree with

  (let ((compat--recordp (if (fboundp #'recordp)
                             #'recordp
                           #'always))
    ...)

and replaced (recordp ...) with (funcall compat--recordp ...).

The backtrace now shows an error at (length (setq tree (copy-sequence tree))).

[minad]

@josephmturner Thanks. I am trying to investigate the problem now.

[phikal]

Joseph Turner ***@***.***> writes:

I have wrapped the body of `compat--copy-tree` with ``` (let ((compat--recordp (if (fboundp #'recordp) #'recordp #'always)) ...) ```

There is no reason to make this complicated like this, just do (or (not (fboundp 'recordp)) (recordp ...)) Also, remember not to sharp-quote a function symbol you are checking with `fboundp', because the point is that it might very well not be bound.

and replaced `(recordp ...)` with `(funcall compat--recordp ...)`. The backtrace [now shows an error](https://github.com/emacs-compat/compat/actions/runs/5044001888/jobs/9046525846?pr=25#step:6:41) at `(length (setq tree (copy-sequence tree)))`.

Related to this, I am also not a fan of using `cl-labels' here either. A little bit of duplication is the safer tactic, IMO.

[minad]

I merged this for now, with some more basic tests added, avoiding the printed representation. Regarding the fboundp check I would like to avoid this in the first place, but this requires some modification of compat-defun

[josephmturner]

Also, remember not to sharp-quote a function symbol you are checking with `fboundp', because the point is that it might very well not be bound.

Good catch. Thank you!

[josephmturner]

I merged this for now, with some more basic tests added, avoiding the printed representation. Regarding the fboundp check I would like to avoid this in the first place, but this requires some modification of compat-defun

Thank you!! I learned a lot working with you two today :)

[minad]

Joseph, thank you, for the addition to both Emacs proper and Compat!