ruricolist / serapeum

Utilities beyond Alexandria
MIT License
420 stars 41 forks source link

Add COPY-ENOUGH-LIST, SPLICE, NSPLICE, SPLICEF, NSPLICEF #121

Closed phoe closed 2 years ago

phoe commented 2 years ago

Closes #117

Code is commented, documented, roughly tested.

Only one TODO remains in the code, which is support for extensible sequences. (Should we?)

phoe commented 2 years ago

I just realized that splice-list will traverse the list twice - once in copy-enough-list, another inside nsplice. It needs an implementation that won't do that.

phoe commented 2 years ago

All done, along with a new implementation for splice-list which only traverses once. i ended up not using firstn-copy in the end - should it be retained since it could be useful on its own, or should it be removed?

phoe commented 2 years ago

Thanks for the review and merge.

ruricolist commented 2 years ago

I found (and fixed) a couple of bugs playing around with this. nsplice-subseq could break on vectors with fill pointers. And splice-list dropped the tail if there were no new arguments. I ended up just rewriting splice-list to accumulate into a queue.

phoe commented 2 years ago

Oops, sloppy testing from my side. Apologies, and thanks again.

phoe commented 2 years ago

I ended up just rewriting splice-list to accumulate into a queue.

It seems to me that the queue implementation will unnecessarily traverse the shared tail of the list because of the last call in qconc that searches for the last element of the tail:

https://github.com/ruricolist/serapeum/blob/c0a563f746fe5ce8f34ff07c24cee9c4b9c1ae0b/queue.lisp#L186-L195

I explicitly tried to optimize my code to avoid this behavior, hence the manual cons-mangling in my original implementation. Should I try to fix up my original code to avoid the bugs you mentioned, or do we not care about cons-perfect optimality here?

Edit: alternatively, should the queue export an operator like qconcret that appends to the tail and immediately returns the head without the unnecessary traversal, clearing the queue in the process?

ruricolist commented 2 years ago

I like the idea of an additional queue operator.

phoe commented 2 years ago

OK, I guess that works. Would you prefer to do it, since I don't know much of the queueing code, or should I submit a PR that adds that operator and modifies SPLICE-LIST?

ruricolist commented 2 years ago

I mean, I like the idea of an additional queue operator on its own merits, and will probably add it at some point whether or not it ends up being used here. In the meantime, if you would prefer to rework the original code, I'm fine with that too.

phoe commented 2 years ago

OK - I'll do both then and submit a PR.

ruricolist commented 2 years ago

It occurred to me that this would be a good application for generative testing, so I added one that compares splice-seq and nsplice-seq, on lists and vectors, with random offsets and subseqs.

phoe commented 2 years ago

Hah! Just like what we've done in split-sequence. Nice.