GNU bug report logs -
#73431
Add `setf` support for `stream.el` in ELPA
Previous Next
Reported by: Okamsn <okamsn <at> protonmail.com>
Date: Mon, 23 Sep 2024 01:35:01 UTC
Severity: wishlist
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
Bug is archived. No further changes may be made.
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 73431 in the body.
You can then email your comments to 73431 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 23 Sep 2024 01:35:01 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
Okamsn <okamsn <at> protonmail.com>:
New bug report received and forwarded. Copy sent to
bug-gnu-emacs <at> gnu.org.
(Mon, 23 Sep 2024 01:35:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hello,
The attached patch adds `setf` support for `stream-first`,
`stream-rest`, and `seq-elt` for streams. The support for `setf` with
`seq-elt` for streams uses the added support for `stream-first`,
following the definition of `seq-elt` for streams.
Would you like anything changed?
Thank you.
[0001-Add-setf-support-to-stream.el.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 24 Sep 2024 10:16:01 GMT)
Full text and
rfc822 format available.
Message #8 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> Hello,
>
> The attached patch adds `setf` support for `stream-first`,
> `stream-rest`, and `seq-elt` for streams. The support for `setf` with
> `seq-elt` for streams uses the added support for `stream-first`,
> following the definition of `seq-elt` for streams.
>
> Would you like anything changed?
>
> Thank you.
>
> From fed785a332bb335522a4b71ef8a68896f304e1d0 Mon Sep 17 00:00:00 2001
> From: Earl Hyatt <okamsn <at> protonmail.com>
> Date: Sun, 22 Sep 2024 19:23:36 -0400
> Subject: [PATCH] Add setf support to stream.el.
>
> * stream.el (\(setf\ stream-first\), \(setf\ stream-rest\)): Add support to
> `setf' for stream-first and stream-rest.
>
> * stream.el (\(setf\ seq-elt\)): Support `setf' with `seq-elt' for streams.
>
> * tests/stream-tests.el (setf-stream-first, setf-stream-first-error)
> (setf-stream-rest, setf-stream-rest-error, setf-stream-seq-elt)
> (setf-stream-seq-elt-error): Add tests for the above features.
> ---
> stream.el | 23 ++++++++++++++++
> tests/stream-tests.el | 64 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 87 insertions(+)
>
> diff --git a/stream.el b/stream.el
> index 7135ee0..eb8b179 100644
> --- a/stream.el
> +++ b/stream.el
> @@ -212,11 +212,23 @@ (defun stream-first (stream)
> Return nil if STREAM is empty."
> (car (stream--force stream)))
>
> +(defun \(setf\ stream-first\) (store stream)
> + "Set the first element of STREAM to value STORE."
> + (if (stream-empty-p stream)
> + (error "Cannot set first element of empty stream: %s" stream)
> + (setcar (stream--force stream) store)))
I am not sure what the preferred practice to define generalised setters
is. In gv.el everything is defined using `gv-define-simple-setter' or
`gv-define-setter', which /feels/ more robust? I believe that Stefan
(as the author or gv.el) might be able to explain if this is so or not.
> +
> (defun stream-rest (stream)
> "Return a stream of all but the first element of STREAM."
> (or (cdr (stream--force stream))
> (stream-empty)))
>
> +(defun \(setf\ stream-rest\) (new-stream stream)
> + "Set the remainder of STREAM to NEW-STREAM."
> + (if (stream-empty-p stream)
> + (error "Cannot set remainder of empty stream: %s" stream)
> + (setcdr (stream--force stream) new-stream)))
> +
> (defun stream-append (&rest streams)
> "Concatenate the STREAMS.
> Requesting elements from the resulting stream will request the
> @@ -273,6 +285,17 @@ (cl-defmethod seq-elt ((stream stream) n)
> (setq n (1- n)))
> (stream-first stream))
>
> +(cl-defmethod \(setf\ seq-elt\) (store (stream stream) n)
> + "Set the element of STREAM at index N to value STORE."
> + (let ((stream-for-signal stream)
> + (n-for-signal n))
> + (while (> n 0)
> + (setq stream (stream-rest stream))
> + (setq n (1- n)))
> + (if (stream-empty-p stream)
> + (signal 'args-out-of-range (list stream-for-signal n-for-signal))
> + (setf (stream-first stream) store))))
> +
> (cl-defmethod seq-length ((stream stream))
> "Return the length of STREAM.
> This function will eagerly consume the entire stream."
> diff --git a/tests/stream-tests.el b/tests/stream-tests.el
> index ba304f1..f82c206 100644
> --- a/tests/stream-tests.el
> +++ b/tests/stream-tests.el
> @@ -308,5 +308,69 @@ (deftest-for-delayed-evaluation (stream-scan #'* 1 (make-delayed-test-stream)))
> (deftest-for-delayed-evaluation (stream-concatenate (stream (list (make-delayed-test-stream)
> (make-delayed-test-stream)))))
>
> +;; Test `setf' support
> +(ert-deftest setf-stream-first ()
> + (should (= 100 (let ((test (stream (vector 0 1 2 3 4))))
> + (setf (stream-first test) 100)
> + (stream-first test))))
> +
> + (should (= 100 (let ((test (stream-range 0 10 2)))
> + (setf (stream-first test) 100)
> + (stream-first test)))))
> +
> +(ert-deftest setf-stream-first-error ()
> + (should-error (let ((test (stream-empty)))
> + (setf (stream-first test) 100)
> + (stream-first test))))
> +
> +(ert-deftest setf-stream-rest ()
> + (should (equal '(0 11 12 13 14)
> + (let ((test (stream (vector 0 1 2 3 4))))
> + (setf (stream-rest test) (stream (list 11 12 13 14)))
> + (seq-into test 'list))))
> +
> + (should (equal '(0 11 12 13 14)
> + (let ((test (stream-range 0 10 2)))
> + (setf (stream-rest test) (stream (list 11 12 13 14)))
> + (seq-into test 'list))))
> +
> + (should (equal '(0 11 12 13 14)
> + (let ((test (stream-range 0 10 2)))
> + ;; Test using an evaluated stream.
> + (setf (stream-rest test)
> + (let ((stream (stream (list 11 12 13 14))))
> + (seq-do #'ignore stream)
> + stream))
> + (seq-into test 'list)))))
> +
> +(ert-deftest setf-stream-rest-error ()
> + (should-error (let ((test (stream-empty)))
> + (setf (stream-rest test) (stream (list 11 12 13 14)))
> + (seq-into test 'list))))
> +
> +(ert-deftest setf-stream-seq-elt ()
> + (should (= 100 (let ((test (stream (vector 0 1 2 3 4))))
> + (setf (seq-elt test 3) 100)
> + (seq-elt test 3))))
> +
> + (should (equal '(0 1 2 100 4)
> + (let ((test (stream (vector 0 1 2 3 4))))
> + (setf (seq-elt test 3) 100)
> + (seq-into test 'list))))
> +
> + (should (= 100 (let ((test (stream-range 0 10 2)))
> + (setf (seq-elt test 3) 100)
> + (seq-elt test 3))))
> +
> + (should (equal '(0 2 4 100 8)
> + (let ((test (stream-range 0 10 2)))
> + (setf (seq-elt test 3) 100)
> + (seq-into test 'list)))))
> +
> +(ert-deftest setf-stream-seq-elt-error ()
> + (should-error (let ((test (stream (vector 0 1 2 3 4))))
> + (setf (seq-elt test 1000) 100))
> + :type 'args-out-of-range))
> +
> (provide 'stream-tests)
> ;;; stream-tests.el ends here
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 24 Sep 2024 13:57:01 GMT)
Full text and
rfc822 format available.
Message #11 received at 73431 <at> debbugs.gnu.org (full text, mbox):
>> +(defun \(setf\ stream-first\) (store stream)
>> + "Set the first element of STREAM to value STORE."
>> + (if (stream-empty-p stream)
>> + (error "Cannot set first element of empty stream: %s" stream)
>> + (setcar (stream--force stream) store)))
>
> I am not sure what the preferred practice to define generalised setters
> is. In gv.el everything is defined using `gv-define-simple-setter' or
> `gv-define-setter', which /feels/ more robust? I believe that Stefan
> (as the author or gv.el) might be able to explain if this is so or not.
Defining \(setf\ FOO\) looks fine to me 🙂
I'm not sure we want to make streams mutable, OTOH.
Is there a known use-case for it?
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 25 Sep 2024 00:19:02 GMT)
Full text and
rfc822 format available.
Message #14 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier wrote:
>>> +(defun \(setf\ stream-first\) (store stream)
>>> + "Set the first element of STREAM to value STORE."
>>> + (if (stream-empty-p stream)
>>> + (error "Cannot set first element of empty stream: %s" stream)
>>> + (setcar (stream--force stream) store)))
>>
>> I am not sure what the preferred practice to define generalised setters
>> is. In gv.el everything is defined using `gv-define-simple-setter' or
>> `gv-define-setter', which /feels/ more robust? I believe that Stefan
>> (as the author or gv.el) might be able to explain if this is so or not.
>
> Defining \(setf\ FOO\) looks fine to me 🙂
> I'm not sure we want to make streams mutable, OTOH.
> Is there a known use-case for it?
>
>
> Stefan
>
Hello,
Currently, using `(setf (seq-elt STREAM 0) VAL)` silently fails, because
it treats the stream as a list, breaking the stream.
On the desire for mutability, there is the included macro `stream-pop`.
My use case is mainly consistency. I am currently cleaning up support
for destructuring generic sequences with generalized variables in my
package, which is how I noticed the silent failure for streams. I have
found streams useful for iterating over sub-sequences of vectors, like
what `cl-maplist` does with lists.
Thank you.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 25 Sep 2024 02:57:01 GMT)
Full text and
rfc822 format available.
Message #17 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> Currently, using `(setf (seq-elt STREAM 0) VAL)` silently fails, because
> it treats the stream as a list, breaking the stream.
Sounds like a bug, indeed. But I'd rather fix it by making it fail
cleanly, to preserve the (current) immutability of streams (at least
until we decide that there's a good reason for streams to be mutable).
> On the desire for mutability, there is the included macro `stream-pop`.
`stream-pop` does not mutate the stream. It only mutates a local
variable (which holds a (reference to a) stream).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 25 Sep 2024 21:40:02 GMT)
Full text and
rfc822 format available.
Message #20 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> Currently, using `(setf (seq-elt STREAM 0) VAL)` silently fails, because
>> it treats the stream as a list, breaking the stream.
>
> Sounds like a bug, indeed. But I'd rather fix it by making it fail
> cleanly, to preserve the (current) immutability of streams (at least
> until we decide that there's a good reason for streams to be mutable).
One exception to the immutability of stream might be buffers? Or at
least it seems like something that would be useful to have.
>> On the desire for mutability, there is the included macro `stream-pop`.
>
> `stream-pop` does not mutate the stream. It only mutates a local
> variable (which holds a (reference to a) stream).
>
>
> Stefan
>
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Thu, 26 Sep 2024 13:54:01 GMT)
Full text and
rfc822 format available.
Message #23 received at 73431 <at> debbugs.gnu.org (full text, mbox):
>> Sounds like a bug, indeed. But I'd rather fix it by making it fail
>> cleanly, to preserve the (current) immutability of streams (at least
>> until we decide that there's a good reason for streams to be mutable).
> One exception to the immutability of stream might be buffers?
Sorry, I don't follow. What do you mean by that?
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 27 Sep 2024 15:13:02 GMT)
Full text and
rfc822 format available.
Message #26 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> Sounds like a bug, indeed. But I'd rather fix it by making it fail
>>> cleanly, to preserve the (current) immutability of streams (at least
>>> until we decide that there's a good reason for streams to be mutable).
>> One exception to the immutability of stream might be buffers?
>
> Sorry, I don't follow. What do you mean by that?
Using (stream (current-buffer)) i create a stream of things in the
current buffer. E.g. using
(seq-find
(lambda (line)
(and line (string-match-p "seq" line)))
(stream (current-buffer) nil 'defun))
I can try to find the first top level definition that contains a
substring (the need to check if the value is non-nil is a bit annoying).
Being able to modify the head of a buffer-stream using setf seems like
something that could be useful, and certainly more efficient than what
many people want to do with splitting the return value of
(buffer-string).
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 27 Sep 2024 16:15:02 GMT)
Full text and
rfc822 format available.
Message #29 received at 73431 <at> debbugs.gnu.org (full text, mbox):
>>>> Sounds like a bug, indeed. But I'd rather fix it by making it fail
>>>> cleanly, to preserve the (current) immutability of streams (at least
>>>> until we decide that there's a good reason for streams to be mutable).
>>> One exception to the immutability of stream might be buffers?
>>
>> Sorry, I don't follow. What do you mean by that?
>
> Using (stream (current-buffer)) i create a stream of things in the
> current buffer. E.g. using
>
> (seq-find
> (lambda (line)
> (and line (string-match-p "seq" line)))
> (stream (current-buffer) nil 'defun))
>
> I can try to find the first top level definition that contains a
> substring (the need to check if the value is non-nil is a bit annoying).
>
> Being able to modify the head of a buffer-stream using setf seems like
> something that could be useful, and certainly more efficient than what
> many people want to do with splitting the return value of
> (buffer-string).
Ah, I see. From afar I can see why that could make sense.
But I can't see how it can fit into the current `stream.el` API and the
proposed `setf`: there is no infrastructure I can see to make it
possible to keep the stream object in sync with modifications made to
the buffer, nor to keep the buffer in sync with modifications made to
the stream.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 27 Sep 2024 21:11:02 GMT)
Full text and
rfc822 format available.
Message #32 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>>>> Sounds like a bug, indeed. But I'd rather fix it by making it fail
>>>>> cleanly, to preserve the (current) immutability of streams (at least
>>>>> until we decide that there's a good reason for streams to be mutable).
>>>> One exception to the immutability of stream might be buffers?
>>>
>>> Sorry, I don't follow. What do you mean by that?
>>
>> Using (stream (current-buffer)) i create a stream of things in the
>> current buffer. E.g. using
>>
>> (seq-find
>> (lambda (line)
>> (and line (string-match-p "seq" line)))
>> (stream (current-buffer) nil 'defun))
>>
>> I can try to find the first top level definition that contains a
>> substring (the need to check if the value is non-nil is a bit annoying).
>>
>> Being able to modify the head of a buffer-stream using setf seems like
>> something that could be useful, and certainly more efficient than what
>> many people want to do with splitting the return value of
>> (buffer-string).
>
> Ah, I see. From afar I can see why that could make sense.
>
> But I can't see how it can fit into the current `stream.el` API and the
> proposed `setf`: there is no infrastructure I can see to make it
> possible to keep the stream object in sync with modifications made to
> the buffer, nor to keep the buffer in sync with modifications made to
> the stream.
Yeah, looking at it again, I don't see an easy way around that either,
so just disregard my comment.
Returning back to the bug report, that means that we should probably
just always handle setf'ing any element in a stream as an error, right?
>
> Stefan
>
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 27 Sep 2024 21:11:02 GMT)
Full text and
rfc822 format available.
Message #35 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> Returning back to the bug report, that means that we should probably
> just always handle setf'ing any element in a stream as an error, right?
That's my opinion, at least, yes.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 27 Sep 2024 23:56:01 GMT)
Full text and
rfc822 format available.
Message #38 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Hi,
I agree to what Stefan is saying.
Philip Kaludercic <philipk <at> posteo.net> writes:
> Being able to modify the head of a buffer-stream using setf seems like
> something that could be useful, and certainly more efficient than what
> many people want to do with splitting the return value of
> (buffer-string).
AFAIK, what you normally do in this situation is creating a new stream
reusing the tail of the given one, like in this toy example:
#+begin_src emacs-lisp
(cl-labels ((integers (&optional from)
(let ((from (or from 1)))
(stream-cons from (integers (1+ from))))))
(let ((my-natural-numbers (integers 1))) ;a stream of the natural numbers: 1, 2, ...
(let ((my-natural-numbers-with-head-replaced ;let's "replace" the first element:
(stream-cons 'not-1 (stream-rest my-natural-numbers))))
;; Test: what are the first 10 elements of this second stream?
(seq-into
(seq-subseq my-natural-numbers-with-head-replaced 0 10)
'list))))
#+end_src
Modifying elements later in the stream conflicts a bit with the idea of
a delayed data structure. The above idea can be modified to work in
this case too, though. Creating a new stream even makes more
transparent what is going on... I don't want to tell anybody how to
work with these guys, but that's how I learned it at university.
In typical scenarios the elements before the one to change already have
been processed and been thrown away, and the element you
actually are interested in is the head element most of the time. Like
for a queue.
For buffer contents listing streams I could imagine that one could make
such a thing invalidate itself when the buffer has been modified;
like here [I only added few lines to the
(stream ((buffer buffer) &optional pos)) implementation]:
#+begin_src emacs-lisp
(cl-defmethod my-buffer-chars (buffer &optional pos)
(let ((mods (buffer-modified-tick))) ; added
(with-current-buffer buffer
(unless pos (setq pos (point-min)))
(if (>= pos (point-max))
(stream-empty))
(stream-cons
(with-current-buffer buffer
(save-excursion
(save-restriction
(widen)
(goto-char pos)
(char-after (point)))))
(if (not (eq mods (buffer-modified-tick))) ; added
(error "Buffer has been modified") ; added
(my-buffer-chars buffer (1+ pos)))))))
#+end_src
Already generated elements normally can't "invalidate themselves",
unless you make them functions. But a whole stream that regenerates or
recomputes itself doesn't seem natural to me. You would rather check
whether the stream is still valid _explicitly_ - and if not, just call
the function that returned that stream (most of the time a named
function, like above) again to create a new stream - in the above
example, possibly with an adopted POS argument.
My two cents.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 28 Sep 2024 03:09:02 GMT)
Full text and
rfc822 format available.
Message #41 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier wrote:
>> Returning back to the bug report, that means that we should probably
>> just always handle setf'ing any element in a stream as an error, right?
>
> That's my opinion, at least, yes.
>
>
> Stefan
>
Hello,
Related to my first message, is there a general way to make streams not
confused with lists? I am going through the other features in `seq.el`,
and I have seen that `seq-sort` is also broken for streams, because
someone added a special implementation for lists. It looks like every
time someone improves the situation for lists by adding a specialized
method, that could break the feature for streams if a specialized method
for streams isn't also added.
Is there a major downside to using `cl-defstruct` to define a stream?
Thank you.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 28 Sep 2024 15:26:01 GMT)
Full text and
rfc822 format available.
Message #44 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> Is there a major downside to using `cl-defstruct` to define a stream?
Probably not major, no. Beware: it'll come with several upsides, tho.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 29 Sep 2024 19:32:01 GMT)
Full text and
rfc822 format available.
Message #47 received at 73431 <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Stefan Monnier wrote:
>> Is there a major downside to using `cl-defstruct` to define a stream?
>
> Probably not major, no. Beware: it'll come with several upsides, tho.
>
>
> Stefan
>
Hello,
Please see the attached file. It changes streams to be structs, warns
that streams are not mutable, adds a creation method for arrays that
doesn't create intermediate sub-arrays, and adds some methods for
streams for more of the seq.el functions.
Please let me know what you would like changed.
Thank you.
[0001-Change-stream.el-to-use-structs-instead-of-cons-cell.patch (text/x-patch, attachment)]
Severity set to 'wishlist' from 'normal'
Request was from
Stefan Kangas <stefankangas <at> gmail.com>
to
control <at> debbugs.gnu.org.
(Mon, 30 Sep 2024 01:37:33 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 30 Sep 2024 22:20:02 GMT)
Full text and
rfc822 format available.
Message #52 received at submit <at> debbugs.gnu.org (full text, mbox):
Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text
editors" <bug-gnu-emacs <at> gnu.org> writes:
> Please see the attached file. It changes streams to be structs, warns
> that streams are not mutable, adds a creation method for arrays that
> doesn't create intermediate sub-arrays, and adds some methods for
> streams for more of the seq.el functions.
Thank you for working on this.
> * stream.el (stream): Define the structure using 'cl-defstruct'.
Does changing the internal representation of streams have an effect
on the speed of the run code?
> * stream.el (seq-sort, seq-reverse, seq-concatenate, seq-remove-at-position):
> Add methods that did not work as expected with the generic implementation.
I don't like these, apart from seq-concatenate.
That we have a unified interface for different types of seqs doesn't
mean we must implement every functionality for every type - we can limit
to those where it makes sense. If you have to translate the complete
stream into an intermediate seq type to implement a feature (like
sorting) hints at that it might not make sense. And I think indeed: if
you need to sort any data than you probably should not use streams to
represent them at all. In the rare cases where this really makes sense
conceptually it is even better to do the translation into a different
seq type explicitly. Converting the result back into a delayed list
makes hardly sense. This is inefficient and leads to bad style.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 30 Sep 2024 22:20:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 02 Oct 2024 01:03:01 GMT)
Full text and
rfc822 format available.
Message #58 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen wrote:
> Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text
> editors" <bug-gnu-emacs <at> gnu.org> writes:
>
>> Please see the attached file. It changes streams to be structs, warns
>> that streams are not mutable, adds a creation method for arrays that
>> doesn't create intermediate sub-arrays, and adds some methods for
>> streams for more of the seq.el functions.
>
> Thank you for working on this.
>
>> * stream.el (stream): Define the structure using 'cl-defstruct'.
>
> Does changing the internal representation of streams have an effect
> on the speed of the run code?
I think that it does make it slower. I am trying to test it, and I think
that making records is slower than making cons cells. I think that
accessing the rest of the stream takes longer because the accessors
created by `cl-defstruct` always perform type checking. It seems to take
about twice as long when compared to naively using `car` and `cdr`.
Do you think that it would be better to disable the type checking in the
accessors? If so, would you please share how to do that? The manual
talks about using `(optimize (safety 0))` in a declare form, but it also
seems to say that it cannot be done locally for just the `cl-defstruct`
usage. If it cannot be done, do think it makes sense to use
`make-record` directly, along with custom function to replace the
generated accessors?
> I don't like these, apart from seq-concatenate.
>
> That we have a unified interface for different types of seqs doesn't
> mean we must implement every functionality for every type - we can limit
> to those where it makes sense. If you have to translate the complete
> stream into an intermediate seq type to implement a feature (like
> sorting) hints at that it might not make sense. And I think indeed: if
> you need to sort any data than you probably should not use streams to
> represent them at all. In the rare cases where this really makes sense
> conceptually it is even better to do the translation into a different
> seq type explicitly. Converting the result back into a delayed list
> makes hardly sense. This is inefficient and leads to bad style.
OK, I will remove them. For the sorting, I looked at Scheme's SRFI-41
(https://srfi.schemers.org/srfi-41/srfi-41.html), which says the
Quicksort could be used for its implementation of streams, but I did not
look in detail.
For passing a stream to the creation function `stream`, do you think it
makes sense to make a shallow copy of the stream via `stream-delay`
(similar to `seq-copy`), or do you think it makes sense to return it
unmodified, which is how I've written it currently?
Thank you.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 02 Oct 2024 01:04:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 02 Oct 2024 19:40:01 GMT)
Full text and
rfc822 format available.
Message #64 received at submit <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> Michael Heerdegen wrote:
>> Okamsn via "Bug reports for GNU Emacs, the Swiss army knife of text
>> editors" <bug-gnu-emacs <at> gnu.org> writes:
>>
>>> Please see the attached file. It changes streams to be structs, warns
>>> that streams are not mutable, adds a creation method for arrays that
>>> doesn't create intermediate sub-arrays, and adds some methods for
>>> streams for more of the seq.el functions.
>>
>> Thank you for working on this.
>>
>>> * stream.el (stream): Define the structure using 'cl-defstruct'.
>>
>> Does changing the internal representation of streams have an effect
>> on the speed of the run code?
>
> I think that it does make it slower. I am trying to test it, and I think
> that making records is slower than making cons cells. I think that
> accessing the rest of the stream takes longer because the accessors
> created by `cl-defstruct` always perform type checking. It seems to take
> about twice as long when compared to naively using `car` and `cdr`.
>
> Do you think that it would be better to disable the type checking in the
> accessors? If so, would you please share how to do that? The manual
> talks about using `(optimize (safety 0))` in a declare form, but it also
> seems to say that it cannot be done locally for just the `cl-defstruct`
> usage. If it cannot be done, do think it makes sense to use
> `make-record` directly, along with custom function to replace the
> generated accessors?
We would have to raise the minimum version from 25 to 26 to support
that. It the overhead noticeable, or just measurable?
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Wed, 02 Oct 2024 19:40:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Thu, 03 Oct 2024 00:20:02 GMT)
Full text and
rfc822 format available.
Message #70 received at submit <at> debbugs.gnu.org (full text, mbox):
Philip Kaludercic wrote:
> Okamsn <okamsn <at> protonmail.com> writes:
>> Michael Heerdegen wrote:
>>> Does changing the internal representation of streams have an effect
>>> on the speed of the run code?
>>
>> I think that it does make it slower. I am trying to test it, and I think
>> that making records is slower than making cons cells. I think that
>> accessing the rest of the stream takes longer because the accessors
>> created by `cl-defstruct` always perform type checking. It seems to take
>> about twice as long when compared to naively using `car` and `cdr`.
>>
>> Do you think that it would be better to disable the type checking in the
>> accessors? If so, would you please share how to do that? The manual
>> talks about using `(optimize (safety 0))` in a declare form, but it also
>> seems to say that it cannot be done locally for just the `cl-defstruct`
>> usage. If it cannot be done, do think it makes sense to use
>> `make-record` directly, along with custom function to replace the
>> generated accessors?
>
> It the overhead noticeable, or just measurable?
I’m not sure what counts as “noticeable”.
Using the benchmark macros given at
https://github.com/alphapapa/emacs-package-dev-handbook#benchmarking, I
tested getting the "first" and “rest” of streams, both as fresh streams
and as already evaluated streams.
These are the results I get for a stream from a list using the current
implementation:
| Form | Tot. runtime | # of GCs | Tot. GC runtime |
|--------------------------+--------------+----------+-----------------|
| stream 10 evald: rest | 0.015259 | 0 | 0 |
| stream 10: rest | 0.044525 | 0 | 0 |
| stream 10 evald: first | 0.059650 | 0 | 0 |
| stream 10: first | 0.074379 | 0 | 0 |
| stream 100: rest | 0.132317 | 0 | 0 |
| stream 100 evald: rest | 0.132821 | 0 | 0 |
| stream 100 evald: first | 0.198041 | 0 | 0 |
| stream 100: first | 0.205684 | 0 | 0 |
| stream 1000 evald: rest | 1.249168 | 0 | 0 |
| stream 1000: rest | 1.250730 | 0 | 0 |
| stream 1000 evald: first | 1.835921 | 0 | 0 |
| stream 1000: first | 1.857300 | 0 | 0 |
These are the results I get for a stream from a list using the
struct-based implementation:
| Form | Tot. runtime | # of GCs | Tot. GC runtime |
|--------------------------+--------------+----------+-----------------|
| stream 10 evald: rest | 0.036241 | 0 | 0 |
| stream 10 evald: first | 0.048213 | 0 | 0 |
| stream 10: rest | 0.048221 | 0 | 0 |
| stream 10: first | 0.048285 | 0 | 0 |
| stream 100 evald: rest | 0.312544 | 0 | 0 |
| stream 100: rest | 0.321046 | 0 | 0 |
| stream 100 evald: first | 0.439694 | 0 | 0 |
| stream 100: first | 0.441674 | 0 | 0 |
| stream 1000: rest | 3.032329 | 0 | 0 |
| stream 1000 evald: rest | 3.142683 | 0 | 0 |
| stream 1000: first | 4.113174 | 0 | 0 |
| stream 1000 evald: first | 4.132561 | 0 | 0 |
You can see that the struct-based run times are about twice as long. I
think this is from the extra work done by the accessors. For example,
the type-checking is run multiple times when accessing the “first” and
“rest” slots, because the accessors are also used in `stream--force`.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Thu, 03 Oct 2024 00:20:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 04 Oct 2024 08:56:01 GMT)
Full text and
rfc822 format available.
Message #76 received at submit <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> Philip Kaludercic wrote:
>> Okamsn <okamsn <at> protonmail.com> writes:
>>> Michael Heerdegen wrote:
>>>> Does changing the internal representation of streams have an effect
>>>> on the speed of the run code?
>>>
>>> I think that it does make it slower. I am trying to test it, and I think
>>> that making records is slower than making cons cells. I think that
>>> accessing the rest of the stream takes longer because the accessors
>>> created by `cl-defstruct` always perform type checking. It seems to take
>>> about twice as long when compared to naively using `car` and `cdr`.
>>>
>>> Do you think that it would be better to disable the type checking in the
>>> accessors? If so, would you please share how to do that? The manual
>>> talks about using `(optimize (safety 0))` in a declare form, but it also
>>> seems to say that it cannot be done locally for just the `cl-defstruct`
>>> usage. If it cannot be done, do think it makes sense to use
>>> `make-record` directly, along with custom function to replace the
>>> generated accessors?
>>
>> It the overhead noticeable, or just measurable?
>
> I’m not sure what counts as “noticeable”.
I'd say that real-world code that uses stream.el gets slower. For me
the main value of synthetic benchmarks is only the speed difference in
orders of magnitude and in the number of GC interrupts.
> Using the benchmark macros given at
> https://github.com/alphapapa/emacs-package-dev-handbook#benchmarking, I
> tested getting the "first" and “rest” of streams, both as fresh streams
> and as already evaluated streams.
>
> These are the results I get for a stream from a list using the current
> implementation:
>
> | Form | Tot. runtime | # of GCs | Tot. GC runtime |
> |--------------------------+--------------+----------+-----------------|
> | stream 10 evald: rest | 0.015259 | 0 | 0 |
> | stream 10: rest | 0.044525 | 0 | 0 |
> | stream 10 evald: first | 0.059650 | 0 | 0 |
> | stream 10: first | 0.074379 | 0 | 0 |
> | stream 100: rest | 0.132317 | 0 | 0 |
> | stream 100 evald: rest | 0.132821 | 0 | 0 |
> | stream 100 evald: first | 0.198041 | 0 | 0 |
> | stream 100: first | 0.205684 | 0 | 0 |
> | stream 1000 evald: rest | 1.249168 | 0 | 0 |
> | stream 1000: rest | 1.250730 | 0 | 0 |
> | stream 1000 evald: first | 1.835921 | 0 | 0 |
> | stream 1000: first | 1.857300 | 0 | 0 |
>
> These are the results I get for a stream from a list using the
> struct-based implementation:
>
> | Form | Tot. runtime | # of GCs | Tot. GC runtime |
> |--------------------------+--------------+----------+-----------------|
> | stream 10 evald: rest | 0.036241 | 0 | 0 |
> | stream 10 evald: first | 0.048213 | 0 | 0 |
> | stream 10: rest | 0.048221 | 0 | 0 |
> | stream 10: first | 0.048285 | 0 | 0 |
> | stream 100 evald: rest | 0.312544 | 0 | 0 |
> | stream 100: rest | 0.321046 | 0 | 0 |
> | stream 100 evald: first | 0.439694 | 0 | 0 |
> | stream 100: first | 0.441674 | 0 | 0 |
> | stream 1000: rest | 3.032329 | 0 | 0 |
> | stream 1000 evald: rest | 3.142683 | 0 | 0 |
> | stream 1000: first | 4.113174 | 0 | 0 |
> | stream 1000 evald: first | 4.132561 | 0 | 0 |
>
> You can see that the struct-based run times are about twice as long. I
> think this is from the extra work done by the accessors. For example,
> the type-checking is run multiple times when accessing the “first” and
> “rest” slots, because the accessors are also used in `stream--force`.
Type checking isn't always that bad; Do you see an (easy) way to avoid
type checking from running multiple times?
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 04 Oct 2024 08:56:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 05 Oct 2024 02:45:02 GMT)
Full text and
rfc822 format available.
Message #82 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Philip Kaludercic wrote:
> Okamsn <okamsn <at> protonmail.com> writes:
>
>> Philip Kaludercic wrote:
>>> Okamsn <okamsn <at> protonmail.com> writes:
>>>> Michael Heerdegen wrote:
>>>>> Does changing the internal representation of streams have an effect
>>>>> on the speed of the run code?
>>>>
>>>> I think that it does make it slower. I am trying to test it, and I think
>>>> that making records is slower than making cons cells. I think that
>>>> accessing the rest of the stream takes longer because the accessors
>>>> created by `cl-defstruct` always perform type checking. It seems to take
>>>> about twice as long when compared to naively using `car` and `cdr`.
>>>>
>>>> Do you think that it would be better to disable the type checking in the
>>>> accessors? If so, would you please share how to do that? The manual
>>>> talks about using `(optimize (safety 0))` in a declare form, but it also
>>>> seems to say that it cannot be done locally for just the `cl-defstruct`
>>>> usage. If it cannot be done, do think it makes sense to use
>>>> `make-record` directly, along with custom function to replace the
>>>> generated accessors?
>>>
>>> It the overhead noticeable, or just measurable?
>>
>> I’m not sure what counts as “noticeable”.
>
> I'd say that real-world code that uses stream.el gets slower. For me
> the main value of synthetic benchmarks is only the speed difference in
> orders of magnitude and in the number of GC interrupts.
>
>> ...
>>
>> You can see that the struct-based run times are about twice as long. I
>> think this is from the extra work done by the accessors. For example,
>> the type-checking is run multiple times when accessing the “first” and
>> “rest” slots, because the accessors are also used in `stream--force`.
>
> Type checking isn't always that bad; Do you see an (easy) way to avoid
> type checking from running multiple times?
Please see the attached file. By setting `safety` to 0 and explicitly
checking only once in `stream--force`, we can avoid the multiple checks
when evaluating an unevaluated stream. That helps when going through the
stream the first time, but that still leaves multiple calls to
`stream--force` when iterating. I don't have any ideas for the latter.
Setting safety to 0 is about 15% faster than having the accessors do
type checking, but it still isn't as fast as the current cons-based
implementation. From what I have tried, iterating through nested arrays
seems slower than iterating through a normal list.
[v2-0001-Change-stream.el-to-use-structs-instead-of-cons-c.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 05 Oct 2024 02:45:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 05 Oct 2024 09:15:02 GMT)
Full text and
rfc822 format available.
Message #88 received at submit <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> Philip Kaludercic wrote:
>> Okamsn <okamsn <at> protonmail.com> writes:
>>
>>> Philip Kaludercic wrote:
>>>> Okamsn <okamsn <at> protonmail.com> writes:
>>>>> Michael Heerdegen wrote:
>>>>>> Does changing the internal representation of streams have an effect
>>>>>> on the speed of the run code?
>>>>>
>>>>> I think that it does make it slower. I am trying to test it, and I think
>>>>> that making records is slower than making cons cells. I think that
>>>>> accessing the rest of the stream takes longer because the accessors
>>>>> created by `cl-defstruct` always perform type checking. It seems to take
>>>>> about twice as long when compared to naively using `car` and `cdr`.
>>>>>
>>>>> Do you think that it would be better to disable the type checking in the
>>>>> accessors? If so, would you please share how to do that? The manual
>>>>> talks about using `(optimize (safety 0))` in a declare form, but it also
>>>>> seems to say that it cannot be done locally for just the `cl-defstruct`
>>>>> usage. If it cannot be done, do think it makes sense to use
>>>>> `make-record` directly, along with custom function to replace the
>>>>> generated accessors?
>>>>
>>>> It the overhead noticeable, or just measurable?
>>>
>>> I’m not sure what counts as “noticeable”.
>>
>> I'd say that real-world code that uses stream.el gets slower. For me
>> the main value of synthetic benchmarks is only the speed difference in
>> orders of magnitude and in the number of GC interrupts.
>>
>>> ...
> >>
>>> You can see that the struct-based run times are about twice as long. I
>>> think this is from the extra work done by the accessors. For example,
>>> the type-checking is run multiple times when accessing the “first” and
>>> “rest” slots, because the accessors are also used in `stream--force`.
>>
>> Type checking isn't always that bad; Do you see an (easy) way to avoid
>> type checking from running multiple times?
>
> Please see the attached file. By setting `safety` to 0 and explicitly
> checking only once in `stream--force`, we can avoid the multiple checks
> when evaluating an unevaluated stream. That helps when going through the
> stream the first time, but that still leaves multiple calls to
> `stream--force` when iterating. I don't have any ideas for the latter.
>
> Setting safety to 0 is about 15% faster than having the accessors do
> type checking, but it still isn't as fast as the current cons-based
> implementation. From what I have tried, iterating through nested arrays
> seems slower than iterating through a normal list.
>
> From 98ffcbe184fdbc403afdf5b1c48e77525dc1d476 Mon Sep 17 00:00:00 2001
> From: Earl Hyatt <okamsn <at> protonmail.com>
> Date: Sat, 28 Sep 2024 15:09:10 -0400
> Subject: [PATCH v2] Change 'stream.el' to use structs instead of cons cells.
> Update features.
>
> * stream.el (stream): Define the structure using 'cl-defstruct'. Set
> safety to 0 using `cl-declaim` to avoid checking the type of the argument
> to `stream--force` multiple times. Instead, explicitly check a single time
> in `stream--force`, which must be used inside the public functions anyway.
>
> * stream.el (stream-make): Change to use new structure constructor
> 'stream--make-stream'.
>
> * stream.el (stream--force, stream-first, stream-rest)
> (stream-empty, stream-empty-p): Redefine to use structure slots. Move
> to be closer to the structure definition.
>
> * stream.el (stream-first, stream-rest): Signal an error when trying to use
> these functions as places for 'setf'.
>
> * stream.el (stream--fresh-identifier, stream--evald-identifier):
> Remove now unused definitions.
>
> * stream.el (stream): Add a method that accepts a stream, returning it
> unmodified. This makes mapping across multiple sequences easier.
>
> * stream.el (stream): Add a method that accepts an array and which does not
> create sub-sequences of the array, unlike the implementation for generic
> sequences. This is a bit faster and is a good example of a custom updater
> function.
>
> * stream.el (stream--generalizer, cl-generic-generalizers): Remove
> these specializers from the old, cons-based implementation.
>
> * stream.el (seq-elt): Signal an error when trying to use this function as a
> place for 'setf'.
>
> * stream.el (seq-concatenate): Add methods that did not work as expected with
> the generic implementation.
>
> * tests/stream-tests.el (stream-seq-concatenate-test, stream-seq-mapcat-test)
> (stream-array-test): Add tests for these features.
>
> * tests/stream-tests.el: Test that evaluation is delayed for seq-drop-while
> using deftest-for-delayed-evaluation.
> ---
> stream.el | 213 +++++++++++++++++++++++++++++-------------
> tests/stream-tests.el | 26 ++++++
> 2 files changed, 176 insertions(+), 63 deletions(-)
>
> diff --git a/stream.el b/stream.el
> index 7135ee0..23b8700 100644
> --- a/stream.el
> +++ b/stream.el
> @@ -66,36 +66,135 @@
> (eval-when-compile (require 'cl-lib))
> (require 'seq)
>
> -(eval-and-compile
> - (defconst stream--fresh-identifier '--stream-fresh--
> - "Symbol internally used to streams whose head was not evaluated.")
> - (defconst stream--evald-identifier '--stream-evald--
> - "Symbol internally used to streams whose head was evaluated."))
> +;; Set safety to 0 to avoid checking the type of the argument multiple times
> +;; within `stream--force', which is used frequently.
> +(cl-declaim (optimize (safety 0)))
> +(cl-defstruct (stream (:constructor stream--make-stream)
> + (:predicate streamp)
> + :named)
> +
> + "A lazily evaluated sequence, compatible with the `seq' library's functions."
> +
> + (evaluated--internal
> + nil
> + :type boolean
> + :documentation "Whether the head and tail of the stream are accessible.
> +
> +This value is set to t via the function `stream--force' after it
> +calls the updater function.")
> +
> + (first--internal
> + nil
> + :type (or t null)
Isn't this type just t? Not a proof, but this doesn't signal:
(mapatoms
(lambda (sym)
(when (and (boundp sym)
(not (cl-typep (symbol-value sym) '(or t null))))
(throw 'fail sym))))
> + :documentation "The first element of the stream.")
> +
> + (rest--internal
> + nil
> + :type (or stream null)
> + :documentation "The rest of the stream, which is itself a stream.")
> +
> + (empty--internal
> + nil
> + :type boolean
> + :documentation "Whether the evaluated stream is empty.
> +
> +A stream is empty if the updater function returns nil when
> +`stream--force' evaluates the stream.")
> +
> + (updater--internal
> + nil
> + :type (or function null)
> + :documentation "Function that returns the head and tail of the stream when called.
> +
> +The updater function returns the head and tail in a cons cell.
> +If it returns nil, then the stream is empty and `empty--internal' is
> +set to t. After this function is called, assuming no errors were signaled,
> +`evaluated--internal' is set to t.
> +
> +In the case of the canonical empty stream (see the variable `stream-empty'),
> +this slot is nil."))
> +
> +(defun stream--force (stream)
> + "Evaluate and return the STREAM.
> +
> +If the output of the updater function is nil, then STREAM is
> +marked as empty. Otherwise, the output of the updater function
> +is used to set the head and the tail of the stream."
> + ;; Check explicitly so that we can avoid checking
> + ;; in accessors by setting safety to 0 via `cl-declaim'.
> + (unless (streamp stream)
> + (signal 'wrong-type-argument (list 'stream stream)))
If you are already using cl-lib, you could also make use of `cl-check-type'.
> + (if (stream-evaluated--internal stream)
> + stream
> + (pcase (funcall (stream-updater--internal stream))
> + (`(,head . ,tail)
> + (setf (stream-first--internal stream) head
> + (stream-rest--internal stream) tail))
> + ((pred null)
> + (setf (stream-empty--internal stream) t))
> + (bad-output
> + (error "Bad output from stream updater: %s"
> + bad-output)))
> + (setf (stream-evaluated--internal stream) t)
> + stream))
>
> (defmacro stream-make (&rest body)
> "Return a stream built from BODY.
> -BODY must return nil or a cons cell whose cdr is itself a
> -stream."
> - (declare (debug t))
> - `(cons ',stream--fresh-identifier (lambda () ,@body)))
>
> -(defun stream--force (stream)
Did you change the order of the definitions?
> - "Evaluate and return the first cons cell of STREAM.
> -That value is the one passed to `stream-make'."
> - (cond
> - ((eq (car-safe stream) stream--evald-identifier)
> - (cdr stream))
> - ((eq (car-safe stream) stream--fresh-identifier)
> - (prog1 (setf (cdr stream) (funcall (cdr stream)))
> - ;; identifier is only updated when forcing didn't exit nonlocally
> - (setf (car stream) stream--evald-identifier)))
> - (t (signal 'wrong-type-argument (list 'streamp stream)))))
> +BODY must return a cons cell whose car would be the head of a
> +stream and whose cdr would be the tail of a stream. The cdr must
> +be a stream itself in order to be a valid tail. Alternatively,
> +BODY may return nil, in which case the stream is marked empty
> +when the stream is evaluated."
> + (declare (debug t))
> + `(stream--make-stream :evaluated--internal nil
> + :updater--internal (lambda () ,@body)))
>
> (defmacro stream-cons (first rest)
> "Return a stream built from the cons of FIRST and REST.
> -FIRST and REST are forms and REST must return a stream."
> +
> +FIRST and REST are forms. REST must return a stream."
> (declare (debug t))
> `(stream-make (cons ,first ,rest)))
> +
> +(defconst stream-empty
> + (stream--make-stream :evaluated--internal t
> + :first--internal nil
> + :rest--internal nil
> + :empty--internal t
> + :updater--internal nil)
> + "The empty stream.")
> +
> +(defun stream-empty ()
> + "Return the empty stream."
> + stream-empty)
This definition also appears unchanged? Please try to keep the diff as
small as possible.
> +
> +(defun stream-empty-p (stream)
> + "Return non-nil if STREAM is empty, nil otherwise."
> + (stream-empty--internal (stream--force stream)))
> +
> +(defun stream-first (stream)
> + "Return the first element of STREAM, evaluating if necessary.
> +
> +If STREAM is empty, return nil."
> + (stream-first--internal (stream--force stream)))
> +
> +(defun \(setf\ stream-first\) (_store _stream)
> + "Signal an error when trying to use `setf' on the head of a stream."
> + (error "Streams are not mutable"))
This should also be part of a second patch.
> +(defun stream-rest (stream)
> + "Return the tail of STREAM, evaluating if necessary.
> +
> +If STREAM is empty, return the canonical empty stream."
> + (if (stream-empty-p stream)
> + stream-empty
> + (stream-rest--internal (stream--force stream))))
> +
> +(defun \(setf\ stream-rest\) (_store _stream)
> + "Signal an error when trying to use `setf' on the tail of a stream."
> + (error "Streams are not mutable"))
> +
>
>
> ;;; Convenient functions for creating streams
> @@ -103,6 +202,10 @@ (defmacro stream-cons (first rest)
> (cl-defgeneric stream (src)
> "Return a new stream from SRC.")
>
> +(cl-defmethod stream ((stream stream))
> + "Return STREAM unmodified."
> + stream)
> +
> (cl-defmethod stream ((seq sequence))
> "Return a stream built from the sequence SEQ.
> SEQ can be a list, vector or string."
> @@ -112,6 +215,24 @@ (cl-defmethod stream ((seq sequence))
> (seq-elt seq 0)
> (stream (seq-subseq seq 1)))))
>
> +(cl-defmethod stream ((array array))
> + "Return a stream built from the array ARRAY."
> + (let ((len (length array)))
> + (if (= len 0)
> + (stream-empty)
> + ;; This approach could avoid one level of indirection by setting
> + ;; `stream-updater--internal' directly, but using `funcall' makes for a
> + ;; good example of how to use a custom updater function using the public
> + ;; interface.
> + (let ((idx 0))
> + (cl-labels ((updater ()
> + (if (< idx len)
> + (prog1 (cons (aref array idx)
> + (stream-make (funcall #'updater)))
> + (setq idx (1+ idx)))
> + nil)))
> + (stream-make (funcall #'updater)))))))
> +
> (cl-defmethod stream ((list list))
> "Return a stream built from the list LIST."
> (if (null list)
> @@ -190,33 +311,6 @@ (defun stream-range (&optional start end step)
> (stream-range (+ start step) end step))))
>
>
> -(defun streamp (stream)
> - "Return non-nil if STREAM is a stream, nil otherwise."
> - (let ((car (car-safe stream)))
> - (or (eq car stream--fresh-identifier)
> - (eq car stream--evald-identifier))))
> -
> -(defconst stream-empty (cons stream--evald-identifier nil)
> - "The empty stream.")
> -
> -(defun stream-empty ()
> - "Return the empty stream."
> - stream-empty)
> -
> -(defun stream-empty-p (stream)
> - "Return non-nil if STREAM is empty, nil otherwise."
> - (null (cdr (stream--force stream))))
> -
> -(defun stream-first (stream)
> - "Return the first element of STREAM.
> -Return nil if STREAM is empty."
> - (car (stream--force stream)))
> -
> -(defun stream-rest (stream)
> - "Return a stream of all but the first element of STREAM."
> - (or (cdr (stream--force stream))
> - (stream-empty)))
> -
> (defun stream-append (&rest streams)
> "Concatenate the STREAMS.
> Requesting elements from the resulting stream will request the
> @@ -240,22 +334,7 @@ (defmacro stream-pop (stream)
> `(prog1
> (stream-first ,stream)
> (setq ,stream (stream-rest ,stream))))
> -
>
> -;;; cl-generic support for streams
> -
> -(cl-generic-define-generalizer stream--generalizer
> - 11
> - (lambda (name &rest _)
> - `(when (streamp ,name)
> - 'stream))
> - (lambda (tag &rest _)
> - (when (eq tag 'stream)
> - '(stream))))
> -
> -(cl-defmethod cl-generic-generalizers ((_specializer (eql stream)))
> - "Support for `stream' specializers."
> - (list stream--generalizer))
>
>
> ;;; Implementation of seq.el generic functions
> @@ -273,6 +352,9 @@ (cl-defmethod seq-elt ((stream stream) n)
> (setq n (1- n)))
> (stream-first stream))
>
> +(cl-defmethod (setf seq-elt) (_store (_sequence stream) _n)
> + (error "Streams are not mutable"))
> +
> (cl-defmethod seq-length ((stream stream))
> "Return the length of STREAM.
> This function will eagerly consume the entire stream."
> @@ -417,6 +499,11 @@ (defmacro stream-delay (expr)
> (cl-defmethod seq-copy ((stream stream))
> "Return a shallow copy of STREAM."
> (stream-delay stream))
> +
> +(cl-defmethod seq-concatenate ((_type (eql stream)) &rest sequences)
> + "Make a stream which concatenates each sequence in SEQUENCES."
> + (apply #'stream-append (mapcar #'stream sequences)))
> +
>
>
> ;;; More stream operations
> diff --git a/tests/stream-tests.el b/tests/stream-tests.el
> index ba304f1..defc544 100644
> --- a/tests/stream-tests.el
> +++ b/tests/stream-tests.el
> @@ -212,6 +212,27 @@ (ert-deftest stream-delay-test ()
> (and (equal res1 5)
> (equal res2 5)))))
>
> +(ert-deftest stream-seq-concatenate-test ()
> + (should (streamp (seq-concatenate 'stream (list 1 2) (vector 3 4) (stream (list 5 6)))))
> + (should (equal '(1 2 3 4 5 6)
> + (seq-into (seq-concatenate 'stream
> + (list 1 2)
> + (vector 3 4)
> + (stream (list 5 6)))
> + 'list))))
> +
> +(ert-deftest stream-seq-mapcat-test ()
> + (should (streamp (seq-mapcat #'stream (list (list 1 2)
> + (vector 3 4)
> + (stream (list 5 6)))
> + 'stream)))
> + (should (equal '(1 2 3 4 5 6)
> + (seq-into (seq-mapcat #'stream (list (list 1 2)
> + (vector 3 4)
> + (stream (list 5 6)))
> + 'stream)
> + 'list))))
> +
> (ert-deftest stream-seq-copy-test ()
> (should (streamp (seq-copy (stream-range))))
> (should (= 0 (stream-first (seq-copy (stream-range)))))
> @@ -234,6 +255,10 @@ (ert-deftest stream-list-test ()
> (dolist (list '(nil '(1 2 3) '(a . b)))
> (should (equal list (seq-into (stream list) 'list)))))
>
> +(ert-deftest stream-array-test ()
> + (dolist (arr (list "cat" [0 1 2]))
> + (should (equal arr (seq-into (stream arr) (type-of arr))))))
> +
> (ert-deftest stream-seq-subseq-test ()
> (should (stream-empty-p (seq-subseq (stream-range 2 10) 0 0)))
> (should (= (stream-first (seq-subseq (stream-range 2 10) 0 3)) 2))
> @@ -296,6 +321,7 @@ (deftest-for-delayed-evaluation (stream-append (make-delayed-test-stream) (make
> (deftest-for-delayed-evaluation (seq-take (make-delayed-test-stream) 2))
> (deftest-for-delayed-evaluation (seq-drop (make-delayed-test-stream) 2))
> (deftest-for-delayed-evaluation (seq-take-while #'numberp (make-delayed-test-stream)))
> +(deftest-for-delayed-evaluation (seq-drop-while #'numberp (make-delayed-test-stream)))
> (deftest-for-delayed-evaluation (seq-map #'identity (make-delayed-test-stream)))
> (deftest-for-delayed-evaluation (seq-mapn #'cons
> (make-delayed-test-stream)
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 05 Oct 2024 09:15:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 05 Oct 2024 13:33:02 GMT)
Full text and
rfc822 format available.
Message #94 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> +(cl-defstruct (stream (:constructor stream--make-stream)
> + (:predicate streamp)
> + :named)
> +
> + "A lazily evaluated sequence, compatible with the `seq' library's functions."
> +
> + (evaluated--internal
> + nil
> + :type boolean
> + :documentation "Whether the head and tail of the stream are accessible.
> +
> +This value is set to t via the function `stream--force' after it
> +calls the updater function.")
> +
> + (first--internal
> + nil
> + :type (or t null)
> + :documentation "The first element of the stream.")
> +
> + (rest--internal
> + nil
> + :type (or stream null)
> + :documentation "The rest of the stream, which is itself a stream.")
> +
> + (empty--internal
> + nil
> + :type boolean
> + :documentation "Whether the evaluated stream is empty.
> +
> +A stream is empty if the updater function returns nil when
> +`stream--force' evaluates the stream.")
> +
> + (updater--internal
> + nil
> + :type (or function null)
> + :documentation "Function that returns the head and tail of the stream when called.
Instead of funny field names, I recommend you use something like
`(:conc-name stream--)` so all the automatically-created accessors have
a "--" in their name, declaring them internal.
Also, I wonder: have you tried to stick closer to the original code,
with a structure like
(cl-defstruct (stream ...)
(evald nil :type boolean)
(data nil :type (or function list)))
Was it worse than what you have?
Stefan
PS: We could even be nasty and do things like:
(cl-defstruct (stream ...)
(data nil :type (or function list)))
(cl-defstruct (stream--unevald (:include stream)))
(cl-defstruct (stream--evald (:include stream)))
and then use (aref STREAM 0) to dynamically change the type from
`stream--unevald` to `stream--evald` to avoid storing the `evald` field.
😈
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 06 Oct 2024 00:38:02 GMT)
Full text and
rfc822 format available.
Message #97 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier wrote:
>> +(cl-defstruct (stream (:constructor stream--make-stream)
>> + (:predicate streamp)
>> + :named)
>> +
>> ...
>> + :documentation "Whether the evaluated stream is empty.
>> +
>> +A stream is empty if the updater function returns nil when
>> +`stream--force' evaluates the stream.")
>> +
>> + (updater--internal
>> + nil
>> + :type (or function null)
>> + :documentation "Function that returns the head and tail of the stream when called.
>
> Instead of funny field names, I recommend you use something like
> `(:conc-name stream--)` so all the automatically-created accessors have
> a "--" in their name, declaring them internal.
I will do that. Thank you for pointing me to it.
> Also, I wonder: have you tried to stick closer to the original code,
> with a structure like
>
> (cl-defstruct (stream ...)
> (evald nil :type boolean)
> (data nil :type (or function list)))
>
> Was it worse than what you have?
>
>
> Stefan
>
>
> PS: We could even be nasty and do things like:
>
> (cl-defstruct (stream ...)
> (data nil :type (or function list)))
>
> (cl-defstruct (stream--unevald (:include stream)))
> (cl-defstruct (stream--evald (:include stream)))
>
> and then use (aref STREAM 0) to dynamically change the type from
> `stream--unevald` to `stream--evald` to avoid storing the `evald` field.
> 😈
I tried both versions you suggested. They both are slower than what I
have currently in the patch file.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 06 Oct 2024 01:37:02 GMT)
Full text and
rfc822 format available.
Message #100 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Philip Kaludercic wrote:
>> +(cl-defstruct (stream (:constructor stream--make-stream)
>> + (:predicate streamp)
>> + :named)
>> +
>> + "A lazily evaluated sequence, compatible with the `seq' library's functions."
>> +
>> + (evaluated--internal
>> + nil
>> + :type boolean
>> + :documentation "Whether the head and tail of the stream are accessible.
>> +
>> +This value is set to t via the function `stream--force' after it
>> +calls the updater function.")
>> +
>> + (first--internal
>> + nil
>> + :type (or t null)
>
> Isn't this type just t? Not a proof, but this doesn't signal:
Yes. I have changed it to just `t`.
>> +(defun stream--force (stream)
>> + "Evaluate and return the STREAM.
>> +
>> +If the output of the updater function is nil, then STREAM is
>> +marked as empty. Otherwise, the output of the updater function
>> +is used to set the head and the tail of the stream."
>> + ;; Check explicitly so that we can avoid checking
>> + ;; in accessors by setting safety to 0 via `cl-declaim'.
>> + (unless (streamp stream)
>> + (signal 'wrong-type-argument (list 'stream stream)))
>
> If you are already using cl-lib, you could also make use of `cl-check-type'.
Done.
>> + (if (stream-evaluated--internal stream)
>> + stream
>> + (pcase (funcall (stream-updater--internal stream))
>> + (`(,head . ,tail)
>> + (setf (stream-first--internal stream) head
>> + (stream-rest--internal stream) tail))
>> + ((pred null)
>> + (setf (stream-empty--internal stream) t))
>> + (bad-output
>> + (error "Bad output from stream updater: %s"
>> + bad-output)))
>> + (setf (stream-evaluated--internal stream) t)
>> + stream))
>>
>> (defmacro stream-make (&rest body)
>> "Return a stream built from BODY.
>> -BODY must return nil or a cons cell whose cdr is itself a
>> -stream."
>> - (declare (debug t))
>> - `(cons ',stream--fresh-identifier (lambda () ,@body)))
>>
>> -(defun stream--force (stream)
>
> Did you change the order of the definitions?
Yes. I brought the fundamental features to the top of the file. In the
existing version, things are defined after they are used. I know that is
not an error, but it had me jumping around more to see how it worked.
>> +
>> +(defconst stream-empty
>> + (stream--make-stream :evaluated--internal t
>> + :first--internal nil
>> + :rest--internal nil
>> + :empty--internal t
>> + :updater--internal nil)
>> + "The empty stream.")
>> +
>> +(defun stream-empty ()
>> + "Return the empty stream."
>> + stream-empty)
>
> This definition also appears unchanged? Please try to keep the diff as
> small as possible.
OK. I have broken it into several files, which I've attached. How does
it look now?
[0004-Add-an-implementation-of-seq-concatenate-for-streams.patch (text/x-patch, attachment)]
[0002-Add-more-efficient-method-for-making-streams-from-ar.patch (text/x-patch, attachment)]
[0003-Add-generalized-variables-for-streams-that-error-whe.patch (text/x-patch, attachment)]
[0001-Change-stream.el-to-use-structures-instead-of-cons-c.patch (text/x-patch, attachment)]
[0005-Add-test-for-delayed-evaluation-for-seq-drop-while-f.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 06 Oct 2024 01:37:03 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 19 Oct 2024 01:01:02 GMT)
Full text and
rfc822 format available.
Message #106 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Okamsn wrote:
> Philip Kaludercic wrote:
>> This definition also appears unchanged? Please try to keep the diff as
>> small as possible.
>
> OK. I have broken it into several files, which I've attached. How does
> it look now?
Hello,
In my previous message, I broke down the changes I made into 5 small
files. Did that improve the clarity for you? Are there other changes
that you would like for me to make?
Thank you.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 19 Oct 2024 01:03:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 19 Oct 2024 10:42:01 GMT)
Full text and
rfc822 format available.
Message #112 received at submit <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> Okamsn wrote:
>> Philip Kaludercic wrote:
>>> This definition also appears unchanged? Please try to keep the diff as
>>> small as possible.
>>
>> OK. I have broken it into several files, which I've attached. How does
>> it look now?
>
> Hello,
>
> In my previous message, I broke down the changes I made into 5 small
> files. Did that improve the clarity for you? Are there other changes
> that you would like for me to make?
Sorry, I have just been behind schedule because of other
responsibilities. My apologies for the delay!
I have taken a look at all the patches, and they look good to me. If
there are no objections, I'd go ahead and apply them in the next few days.
--
Philip Kaludercic on siskin
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sat, 19 Oct 2024 10:42:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 21 Oct 2024 15:49:01 GMT)
Full text and
rfc822 format available.
Message #118 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> In my previous message, I broke down the changes I made into 5 small
> files. Did that improve the clarity for you? Are there other changes
> that you would like for me to make?
> * stream.el (stream): Define the structure using 'cl-defstruct'. Set
> safety to 0 using 'cl-declaim' to avoid checking the type of the argument
> to 'stream--force' multiple times. Instead, explicitly check a single time
> in 'stream--force', which must be used inside the public functions anyway.
How much slower or faster is forcing with this change, in the end?
> + (bad-output
> + (error "Bad output from stream updater: %s"
> + bad-output)))
^^
Should this better be %S (we use %s for strings only)?
TIA,
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 21 Oct 2024 15:50:01 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 21 Oct 2024 20:41:02 GMT)
Full text and
rfc822 format available.
Message #124 received at submit <at> debbugs.gnu.org (full text, mbox):
>> + (bad-output
>> + (error "Bad output from stream updater: %s"
>> + bad-output)))
> ^^
>
> Should this better be %S (we use %s for strings only)?
+1
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 21 Oct 2024 20:41:03 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 22 Oct 2024 13:13:01 GMT)
Full text and
rfc822 format available.
Message #130 received at submit <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen via "Bug reports for GNU Emacs, the Swiss army knife
of text editors" <bug-gnu-emacs <at> gnu.org> writes:
> How much slower or faster is forcing with this change, in the end?
I now tried el-search using your patch. Everything worked well and so
far I did not see any obvious performance degradation.
> > + (bad-output
> > + (error "Bad output from stream updater: %s"
> > + bad-output)))
> ^^
>
> Should this better be %S (we use %s for strings only)?
Also: when compiling using master I get
| stream.el:395:15: Warning: docstring wider than 80 characters
| stream.el:421:15: Warning: docstring has wrong usage of unescaped single
| quotes (use \=' or different quoting such as `...')
Could you please try to care about these?
But apart from these details your patches look fine to me. Thanks for
working on this.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 22 Oct 2024 13:13:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Thu, 24 Oct 2024 02:52:02 GMT)
Full text and
rfc822 format available.
Message #136 received at submit <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen wrote:
> Okamsn <okamsn <at> protonmail.com> writes:
>> * stream.el (stream): Define the structure using 'cl-defstruct'. Set
>> safety to 0 using 'cl-declaim' to avoid checking the type of the
argument
>> to 'stream--force' multiple times. Instead, explicitly check a
single time
>> in 'stream--force', which must be used inside the public functions
anyway.
>
> How much slower or faster is forcing with this change, in the end?
In my tests of iterating through the stream, the increase in speed from
disabling the safety checks ranged from about 10% to about 20%.
>
>
>> + (bad-output
>> + (error "Bad output from stream updater: %s"
>> + bad-output)))
> ^^
>
> Should this better be %S (we use %s for strings only)?
I have fixed this in the local version. I will send an updated set of
patches after a better doc string for `seq-take-while` is chosen, to
reduce noise.
Michael Heerdegen wrote:
> Also: when compiling using master I get
>
> | stream.el:395:15: Warning: docstring wider than 80 characters
> | stream.el:421:15: Warning: docstring has wrong usage of unescaped single
> | quotes (use \=' or different quoting such as `...')
>
> Could you please try to care about these?
For shortening the first line of the documentation of `seq-take-while`,
do you think changing "Return a stream of the successive elements for
which (PRED elt) is non-nil in STREAM" to "Return a stream of serial
elements in STREAM for which PRED returns non-nil" works? Also, do you
think that the documentation string for `seq-drop-while` should also be
changed for consistency?
> But apart from these details your patches look fine to me. Thanks for
> working on this.
Thank you for testing the patch with el-search.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Thu, 24 Oct 2024 02:52:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 27 Oct 2024 10:07:02 GMT)
Full text and
rfc822 format available.
Message #142 received at submit <at> debbugs.gnu.org (full text, mbox):
Okamsn <okamsn <at> protonmail.com> writes:
> For shortening the first line of the documentation of `seq-take-while`,
> do you think changing "Return a stream of the successive elements for
> which (PRED elt) is non-nil in STREAM" to "Return a stream of serial
> elements in STREAM for which PRED returns non-nil" works?
Hmm. How about
"Return the starting consecutive elements that fulfill PRED."
Or "front elements"?
I tried to emphasize that the function is not about the whole sequence
but starts at the front and aborts once PRED is not fulfilled. We can
later say explicitly that the predicate is called like (PRED ELT) - that
alone makes the sentence shorter.
But I'm not that good when using English language - better versions
welcome.
> Also, do you think that the documentation string for `seq-drop-while`
> should also be changed for consistency?
Sure.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 27 Oct 2024 10:07:02 GMT)
Full text and
rfc822 format available.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Sun, 27 Oct 2024 14:28:02 GMT)
Full text and
rfc822 format available.
Message #148 received at 73431 <at> debbugs.gnu.org (full text, mbox):
>> | stream.el:395:15: Warning: docstring wider than 80 characters
>> | stream.el:421:15: Warning: docstring has wrong usage of unescaped single
>> | quotes (use \=' or different quoting such as `...')
>>
>> Could you please try to care about these?
>
> For shortening the first line of the documentation of `seq-take-while`,
> do you think changing "Return a stream of the successive elements for
> which (PRED elt) is non-nil in STREAM" to "Return a stream of serial
> elements in STREAM for which PRED returns non-nil" works?
Why do we even need a docstring? The generic function already comes
with its docstring:
Take the successive elements of SEQUENCE for which PRED returns non-nil.
PRED is a function of one argument. The function keeps collecting
elements from SEQUENCE and adding them to the result until PRED
returns nil for an element.
Value is a sequence of the same type as SEQUENCE.
Methods of a generic function shouldn't duplicate the generic
function's docstring. They may add some clarifications specific to the
method, of course, but in most cases no docstring is needed.
Look at what your docstring does in `C-h f seq-take-while` to judge
whether it's appropriate.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Mon, 28 Oct 2024 09:43:02 GMT)
Full text and
rfc822 format available.
Message #151 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> > For shortening the first line of the documentation of `seq-take-while`
> > [...]
> Why do we even need a docstring? The generic function already comes
> with its docstring: [...]
I agree 100%.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 01:16:02 GMT)
Full text and
rfc822 format available.
Message #154 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen wrote:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> For shortening the first line of the documentation of `seq-take-while`
>>> [...]
>
>> Why do we even need a docstring? The generic function already comes
>> with its docstring: [...]
>
> I agree 100%.
>
>
> Michael.
Hello,
Did you also want to delete the documentation string of `seq-mapn`,
which contains an example?
Thank you.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 02:01:01 GMT)
Full text and
rfc822 format available.
Message #157 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> Did you also want to delete the documentation string of `seq-mapn`,
> which contains an example?
I can't find the patch right now to make an informed judgment, but the
docstring of the stream method of `seq-mapn` should be specific to the
stream method.
Please look at the result in `C-h f seq-mapn` to see if your
docstring makes sense.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 09:58:02 GMT)
Full text and
rfc822 format available.
Message #160 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> > Did you also want to delete the documentation string of `seq-mapn`,
> > which contains an example?
>
> I can't find the patch right now to make an informed judgment, but the
> docstring of the stream method of `seq-mapn` should be specific to the
> stream method.
> Please look at the result in `C-h f seq-mapn` to see if your
> docstring makes sense.
Dunno how this could be beautified to harmonize better with the C-h f
output BUT we definitely do want to keep the Fibonacci number
construction example because it is very educative. And it fits well
here in this docstring, so, modulo cosmetic improvements, I would let it
be.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 10:35:01 GMT)
Full text and
rfc822 format available.
Message #163 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Michael Heerdegen <michael_heerdegen <at> web.de> writes:
> > Please look at the result in `C-h f seq-mapn` to see if your
> > docstring makes sense.
> [...] we definitely do want to keep the Fibonacci number
> construction example because it is very educative.
Is it allowed for method docstrings that the first line is not a
complete sentence with these and that requirements? Then I would remove
the first, redundant and thus confusing sentence (doesn't tell anything
about this implementation) and use a docstring like
"Example:
(the Fibo example ...)"
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 12:44:01 GMT)
Full text and
rfc822 format available.
Message #166 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> Cc: Okamsn <okamsn <at> protonmail.com>, philipk <at> posteo.net, nicolas <at> petton.fr,
> 73431 <at> debbugs.gnu.org
> Date: Tue, 29 Oct 2024 11:35:10 +0100
> From: Michael Heerdegen via "Bug reports for GNU Emacs,
> the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>
> Is it allowed for method docstrings that the first line is not a
> complete sentence with these and that requirements? Then I would remove
> the first, redundant and thus confusing sentence (doesn't tell anything
> about this implementation) and use a docstring like
>
> "Example:
>
> (the Fibo example ...)"
The rule that the first line should be a complete sentence (actually,
a summary) is for the benefit of the apropos commands, which show only
the first line of each doc string. So the answer to your question
depends on whether these methods can wind up in output of apropos
commands, and what happens if and when they do.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 13:31:01 GMT)
Full text and
rfc822 format available.
Message #169 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> The rule that the first line should be a complete sentence (actually,
> a summary) is for the benefit of the apropos commands, which show only
> the first line of each doc string. So the answer to your question
> depends on whether these methods can wind up in output of apropos
> commands, and what happens if and when they do.
Thanks.
At the first glance this doesn't seem the case. We do have this FIXME
in "apropos.el" though:
;; FIXME: Print information about each individual method: both
;; its docstring and specializers (bug#21422).
So I guess we want to follow the rule here. But then - how to provide
the nice example here, without repeating the first sentence of the
generic. Any idea?
Thanks,
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 15:04:01 GMT)
Full text and
rfc822 format available.
Message #172 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> "Example:
> (the Fibo example ...)"
Looks OK to me. We could also use an empty first line.
> ;; FIXME: Print information about each individual method: both
> ;; its docstring and specializers (bug#21422).
We'll cross that bridge when we get there.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 15:06:01 GMT)
Full text and
rfc822 format available.
Message #175 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> Dunno how this could be beautified to harmonize better with the C-h f
> output BUT we definitely do want to keep the Fibonacci number
> construction example because it is very educative. And it fits well
> here in this docstring, so, modulo cosmetic improvements, I would let it
> be.
Maybe such examples would be better in a shortdoc.
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 15:44:02 GMT)
Full text and
rfc822 format available.
Message #178 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: monnier <at> iro.umontreal.ca, okamsn <at> protonmail.com, philipk <at> posteo.net,
> nicolas <at> petton.fr, 73431 <at> debbugs.gnu.org
> Date: Tue, 29 Oct 2024 14:31:34 +0100
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > The rule that the first line should be a complete sentence (actually,
> > a summary) is for the benefit of the apropos commands, which show only
> > the first line of each doc string. So the answer to your question
> > depends on whether these methods can wind up in output of apropos
> > commands, and what happens if and when they do.
>
> Thanks.
>
> At the first glance this doesn't seem the case. We do have this FIXME
> in "apropos.el" though:
>
> ;; FIXME: Print information about each individual method: both
> ;; its docstring and specializers (bug#21422).
>
> So I guess we want to follow the rule here. But then - how to provide
> the nice example here, without repeating the first sentence of the
> generic. Any idea?
Can you point me to the doc strings you are talking about, so I could
see their text and the examples you'd like to keep?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 16:09:01 GMT)
Full text and
rfc822 format available.
Message #181 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> > So I guess we want to follow the rule here. But then - how to provide
> > the nice example here, without repeating the first sentence of the
> > generic. Any idea?
>
> Can you point me to the doc strings you are talking about, so I could
> see their text and the examples you'd like to keep?
We are discussing the docstring of the implementation of `seq-mapn' for
streams in "stream.el" (line 342).
TIA,
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 16:19:01 GMT)
Full text and
rfc822 format available.
Message #184 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> Maybe such examples would be better in a shortdoc.
I dunno, but the example also tells something essential: that the
mapping happens in a delayed way. That, and the implications, might not
be obvious. Maybe it's not wrong to have that in a docstring - maybe
even more explicit than currently?
I'm also looking at the doc of `seq-map's implementation in stream.el.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 16:26:02 GMT)
Full text and
rfc822 format available.
Message #187 received at 73431 <at> debbugs.gnu.org (full text, mbox):
>> Maybe such examples would be better in a shortdoc.
> I dunno, but the example also tells something essential: that the
> mapping happens in a delayed way. That, and the implications, might not
> be obvious. Maybe it's not wrong to have that in a docstring - maybe
> even more explicit than currently?
AFAIK, `seq-map` and `seq-mapn` always return a sequence of the same
type (same type as the first sequence in the case of `seq-mapn`).
Maybe their doc should say so more explicitly.
That info would be sufficient to know that the `seq-map(n)` does its
job lazily when applied to a stream (it would be a clear misfeature to
return a stream and yet to build that stream eagerly).
Stefan
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 17:07:03 GMT)
Full text and
rfc822 format available.
Message #190 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: monnier <at> iro.umontreal.ca, okamsn <at> protonmail.com, philipk <at> posteo.net,
> nicolas <at> petton.fr, 73431 <at> debbugs.gnu.org
> Date: Tue, 29 Oct 2024 17:09:04 +0100
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > > So I guess we want to follow the rule here. But then - how to provide
> > > the nice example here, without repeating the first sentence of the
> > > generic. Any idea?
> >
> > Can you point me to the doc strings you are talking about, so I could
> > see their text and the examples you'd like to keep?
>
> We are discussing the docstring of the implementation of `seq-mapn' for
> streams in "stream.el" (line 342).
Thanks. But I'm not sure I understand the problem. If I load both
seq.el and stream.el, and then type "C-h f seq-mapn", I see this:
seq-mapn is a byte-code-function in ‘seq.el’.
(seq-mapn FUNCTION SEQUENCES...)
Return the result of applying FUNCTION to each element of SEQUENCEs.
Like ‘seq-map’, but FUNCTION is mapped over all SEQUENCEs.
The arity of FUNCTION must match the number of SEQUENCEs, and the
mapping stops on the shortest sequence.
Return a list of the results.
This is a generic function.
Implementations:
(seq-mapn ARG0 (ARG1 stream) &rest CL--ARGS) in ‘~/data/stream-2.3.0/stream.el’.
Map FUNCTION over the STREAMS.
Example: this prints the first ten Fibonacci numbers:
(letrec ((fibs (stream-cons
1
(stream-cons
1
(seq-mapn #’+ fibs (stream-rest fibs))))))
(seq-do #’print (seq-take fibs 10)))
(seq-mapn FUNCTION SEQUENCE &rest SEQUENCES) in ‘seq.el’.
Undocumented
The "Undocumented" part at the end aside, I see both the doc string of
defgeneric and the doc string of the implementation for streams,
complete with the example. The only aspect of this I'd change is the
first line of the doc string for the streams implementation: I'd make
it say
Implementation of `seq-mapn' for streams.
Other than that, everything looks good, and "M-x apropos" shows only
the generic function.
What problem did you try to solve?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 17:29:02 GMT)
Full text and
rfc822 format available.
Message #193 received at 73431 <at> debbugs.gnu.org (full text, mbox):
Eli Zaretskii <eliz <at> gnu.org> writes:
> The only aspect of this I'd change is the first line of the doc string
> for the streams implementation: I'd make it say
>
> Implementation of `seq-mapn' for streams.
>
> Other than that, everything looks good, and "M-x apropos" shows only
> the generic function.
>
> What problem did you try to solve?
Thanks... yes, that problem: how to avoid repeating what the docstring
of the generic already says - because that would suggest that the
implementation does something specific or differently.
But we also need to find out where these lines:
| (seq-mapn FUNCTION SEQUENCE &rest SEQUENCES) in `seq.el'.
|
| Undocumented
come from (the link to the source is broken).
And whether we can avoid these ARG0 ARG1 variable names:
| Implementations:
|
| (seq-mapn ARG0 (ARG1 stream) &rest CL--ARGS)
that are not present in the source.
Michael.
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Tue, 29 Oct 2024 17:51:02 GMT)
Full text and
rfc822 format available.
Message #196 received at 73431 <at> debbugs.gnu.org (full text, mbox):
> From: Michael Heerdegen <michael_heerdegen <at> web.de>
> Cc: monnier <at> iro.umontreal.ca, okamsn <at> protonmail.com, philipk <at> posteo.net,
> nicolas <at> petton.fr, 73431 <at> debbugs.gnu.org
> Date: Tue, 29 Oct 2024 18:29:10 +0100
>
> Eli Zaretskii <eliz <at> gnu.org> writes:
>
> > The only aspect of this I'd change is the first line of the doc string
> > for the streams implementation: I'd make it say
> >
> > Implementation of `seq-mapn' for streams.
> >
> > Other than that, everything looks good, and "M-x apropos" shows only
> > the generic function.
> >
> > What problem did you try to solve?
>
> Thanks... yes, that problem: how to avoid repeating what the docstring
> of the generic already says - because that would suggest that the
> implementation does something specific or differently.
That line comes directly from the doc string in stream.el, so if we
change it to say
Implementation of `seq-mapn' for streams.
the problem will be solved, no?
> But we also need to find out where these lines:
>
> | (seq-mapn FUNCTION SEQUENCE &rest SEQUENCES) in `seq.el'.
> |
> | Undocumented
>
> come from (the link to the source is broken).
>
> And whether we can avoid these ARG0 ARG1 variable names:
>
> | Implementations:
> |
> | (seq-mapn ARG0 (ARG1 stream) &rest CL--ARGS)
>
> that are not present in the source.
Right, but that's a separate issue, I think?
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 08 Nov 2024 01:47:02 GMT)
Full text and
rfc822 format available.
Message #199 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Michael Heerdegen wrote:
> Okamsn <okamsn <at> protonmail.com> writes:
>
>> For shortening the first line of the documentation of `seq-take-while`,
>> do you think changing "Return a stream of the successive elements for
>> which (PRED elt) is non-nil in STREAM" to "Return a stream of serial
>> elements in STREAM for which PRED returns non-nil" works?
>
> Hmm. How about
>
> "Return the starting consecutive elements that fulfill PRED."
>
> Or "front elements"?
>
> I tried to emphasize that the function is not about the whole sequence
> but starts at the front and aborts once PRED is not fulfilled. We can
> later say explicitly that the predicate is called like (PRED ELT) - that
> alone makes the sentence shorter.
>
> But I'm not that good when using English language - better versions
> welcome.
>
>> Also, do you think that the documentation string for `seq-drop-while`
>> should also be changed for consistency?
>
> Sure.
>
>
> Michael.
Hello,
Attached are patch files which include the requested shorter doc strings
for `seq-take-while` and `seq-drop-while`. The quoting in `seq-mapn`
has been fixed. I did not delete any of the doc strings because there
does not seem to be consensus on that.
Is there anything else that you would like changed outside of the
discussion about the doc strings?
Thank you.
[v5-0007-Shorten-the-documentation-strings-of-seq-take-whi.patch (text/x-patch, attachment)]
[v5-0006-Fix-the-quoting-in-the-documentation-of-seq-mapn-.patch (text/x-patch, attachment)]
[v5-0005-Add-test-for-delayed-evaluation-for-seq-drop-whil.patch (text/x-patch, attachment)]
[v5-0004-Add-an-implementation-of-seq-concatenate-for-stre.patch (text/x-patch, attachment)]
[v5-0003-Add-generalized-variables-for-streams-that-error-.patch (text/x-patch, attachment)]
[v5-0002-Add-more-efficient-method-for-making-streams-from.patch (text/x-patch, attachment)]
[v5-0001-Change-stream.el-to-use-structures-instead-of-con.patch (text/x-patch, attachment)]
Information forwarded
to
bug-gnu-emacs <at> gnu.org:
bug#73431; Package
emacs.
(Fri, 08 Nov 2024 01:47:03 GMT)
Full text and
rfc822 format available.
Reply sent
to
Stefan Monnier <monnier <at> iro.umontreal.ca>:
You have taken responsibility.
(Mon, 18 Nov 2024 02:18:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
Okamsn <okamsn <at> protonmail.com>:
bug acknowledged by developer.
(Mon, 18 Nov 2024 02:18:02 GMT)
Full text and
rfc822 format available.
Message #207 received at 73431-done <at> debbugs.gnu.org (full text, mbox):
Not sure who should do it, but since noone did it yet, I went ahead and
pushed it to `elpa.git`. Thanks.
Stefan
Okamsn [2024-11-08 01:45:47] wrote:
> Michael Heerdegen wrote:
>> Okamsn <okamsn <at> protonmail.com> writes:
>>
>>> For shortening the first line of the documentation of `seq-take-while`,
>>> do you think changing "Return a stream of the successive elements for
>>> which (PRED elt) is non-nil in STREAM" to "Return a stream of serial
>>> elements in STREAM for which PRED returns non-nil" works?
>>
>> Hmm. How about
>>
>> "Return the starting consecutive elements that fulfill PRED."
>>
>> Or "front elements"?
>>
>> I tried to emphasize that the function is not about the whole sequence
>> but starts at the front and aborts once PRED is not fulfilled. We can
>> later say explicitly that the predicate is called like (PRED ELT) - that
>> alone makes the sentence shorter.
>>
>> But I'm not that good when using English language - better versions
>> welcome.
>>
>>> Also, do you think that the documentation string for `seq-drop-while`
>>> should also be changed for consistency?
>>
>> Sure.
>>
>>
>> Michael.
>
> Hello,
>
> Attached are patch files which include the requested shorter doc strings
> for `seq-take-while` and `seq-drop-while`. The quoting in `seq-mapn`
> has been fixed. I did not delete any of the doc strings because there
> does not seem to be consensus on that.
>
> Is there anything else that you would like changed outside of the
> discussion about the doc strings?
>
> Thank you.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Mon, 16 Dec 2024 12:24:09 GMT)
Full text and
rfc822 format available.
This bug report was last modified 264 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.