Spencer Baugh <sbaugh@janestreet.com> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Two things:
>>>
>>> - It should have somewhat better performance because we're running
>>> try-completion on smaller strings and therefore doing less string
>>> comparison and work.
>>
>> Do we have concrete evidence to believe this?
>
> No evidence.  So nevermind the performance argument, I suppose.
>
>>> CCS previously contained, for each completion, a list containing one
>>> element for each wildcard element of PATTERN.
>>>
>>> Now CCS contains, for each completion, a list containing one element for
>>> each element of PATTERN, both fixed strings and wildcards.
>>
>> Exactly: that means CCS is more costly to build and AFAICT it also
>> implies correspondingly more calls to `try-completion`.
>>
>> I don't find it obvious that the new code will be more efficient.
>> Barring any clear evidence that one of the two versions is noticeably
>> more efficient than the other, I'd recommend we stick to the code we
>> happen to have.
>>
>> From where I stand, my impression is that neither version is terribly
>> better nor terribly worse than the other, in terms of clarity,
>> simplicity, and efficiency.
>
> That's fair, that patch alone is not worth much.
>
> Attached is a patch (which applies on master) which does most of the
> rest of the refactoring that I wanted to do; it makes the dolist over
> the pattern into a simple mapcan over the pattern, with no state
> preserved between elements of the pattern.  IMO, this is much easier to
> follow (and I think it would make the various bugs I've fixed in the
> last couple years in merge-completions, easier to catch).  What do you
> think?

This patch has a small bug with any-delim, here's a fixed version, now
with a test for that case.