-
Notifications
You must be signed in to change notification settings - Fork 13
Commit
- Loading branch information
There are no files selected for viewing
8 comments
on commit 8190769
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:reverse
isn't handled correctly. If the comparison function is <
, then you can use (lambda (a b) (< b a))
. I prefer reversing the sequence before and after sorting instead because it's faster than adding an indirection to the comparison function.
When sorting lists in-place, Emacs 29 and newer will modify the contents of the original list; this is documented in Emacs 30. This could be replicated in Emacs 28 and older but I'm not sure it would be worth the trouble.
You may want to run (or steal) some of the Emacs 30 tests for value<
and sort
in fns-tests.el
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When sorting lists in-place, Emacs 29 and newer will modify the contents of the original list; this is documented in Emacs 30. This could be replicated in Emacs 28 and older but I'm not sure it would be worth the trouble.
Should be the case now, I think.
Not sure I follow the logic then. What I meant is that the expected result of
(let ((a (list 5 1 3 4 2)))
(sort a :in-place t) ; or (sort a #'<)
a)
is (1 2 3 4 5)
. In Emacs 28 it probably isn't.
To accomplish this, a fully compatible implementation would need to sort a copy of the list, and then copy back each element of the result into the CARs of the original list. May or may not be too much trouble.
And I'm a bit confused by the part
(setq
...
seq (if (or (eval-when-compile (< emacs-major-version 25)) in-place)
seq
(copy-sequence seq))))
Doesn't this force in-place=t for lists in Emacs 24?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I see. What I meant is that the list is modified destructively but it is not made sure that the first cons stays the first. I didn't understand that even this invariant is preserved in newer Emacs.
It was introduced in Emacs 29 entirely for performance reasons but we didn't advertise the change out of prudence. It's actually useful enough that we do imply it in the manual now, maybe not crystal-clearly.
Also I believe sort is marked with important-return-value, to avoid issues due to a change of the first cons cell?
There is some ad-hoc compiler logic in Emacs 30 so that it doesn't warn for in-place sorts.
Yes, there is a bug here I think. I'll push a fix. EDIT: Fixed in [b12b2e5]
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just switched a few days ago to Emacs 30 and everything feels more snappy!
That's probably just the native-compilation that's now on by default. All Andrea's work!
Great! Is this also the case for delq/delete? Would it also be possible to make sure that the first cons stays the first there? This would require some special casing of the case where the first cons is deleted. Maybe there are other list functions which could get a similar treatment?
That's an interesting idea – currently delq
and delete
just skip the prefix of matching elements before doing any mutation at all, which saves some time but how much is unclear. There may be crusty old code relying on the CARs not being changed though.
Right now the important-return-value warning is not enabled for delq
, delete
, nconc
or plist-put
because there were just too many false positives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Numbers and markers don't compare in
value<
, so they would have to be treated separately here. As the doc string says, markers are compared lexicographically by buffer, then position.