Re: Upstreaming parsebib.el

[Philip Kaludercic]

Joost Kremers <joostkremers@fastmail.fm> writes:

> On Fri, Jan 31 2025, Philip Kaludercic wrote:
>>> I'm using string for keys because that's what bibtex.el also uses. It makes
>>> it easier to use functions from bibtex.el on the data, if needs be.
>>> (parsebib.el itself doesn't do this, but Ebib, my bib manager which uses
>>> parsebib.el to read .bib files does).
>>
>> That's fine then.  Was there a comment that explained this somewhere?
>
> Nope, but I'll add one.

1+

>> The idea is to replace
>>
>>   (delq nil (mapcar (lambda (x) (if (fn x) x nil))  list))
>>
>> with
>>
>>   (mapcan (lambda (x) (if (fn x) (list x) nil)) list)
>>
>> as you are providing the cons-cells that the resulting list will be made
>> up of (instead of creating a cons-cell for each element of the old list
>> and then discarding some of those).
>
> Ah, thanks. If it can speed up the parser, then it's a good thing to do.

Yes, though one should be careful with destructuve operations in
general, I think that this is a safe enough pattern that can
meaningfully ease GC.

>> With `pcase-exhaustive' you would raise an error, but my understanding
>> was that this should be interpreted as an invalid state anyway.  So it
>> is better to make it explicit and fail early than continuing on with a
>> nil value.
>
> Actually, this shouldn't error out. It's just for making field values
> prettier to look at, so it'd be better to return the original string if no
> match is found.

Ah, in that case pcase-exhaustive wouldn't be the right thing, I
couldn't evaluate it from just reading over the code.

>>> Is there any reason to prefer `dolist` over `mapc`? (I've never really
>>> understood why both exist, TBH. I used to have an irrational preference for
>>> `mapc`, but I've gotten over that. 😉)
>>
>> I think that from a readability perspective, it is useful to mention
>> what you are looping over at the beginning, but the other point is that
>> it is (slightly) more expensive to funcall a closure than to use
>> `while'.  But again, this is just a suggestion, any strong opinion from
>> your side overrides that :)
>
> I don't have a strong opinion on it. If it can potentially speed up
> parsing, even by a bit, that would be my main criterion.

I would try it out, my instinct would be that it is faster.  (My general
rule for mapc is to only use if it I have a function symbol I can pass
directly.)

>> A different perspective is to ask if there is a hurry to merge this.  As
>> Emacs 31 is  not about to be cut soon, I would say that it should be
>> fine to wait a bit after the matter has been discussed with whoever is
>> responsible for merging parsebib into emacs.git.  Do you know who that
>> is, btw?
>
> AFAIK it hasn't even been decided whether it gets added to core or not. 

I see, then we should probably wait before doing anything rash.