bug#42149: Substring and flex completion ignore implicit trailing ‘any’

[João Távora]

Dario Gjorgjevski <dario.gjorgjevski@gmail.com> writes:

>> Last time I looked it was too complex for me to follow, touches many
>> lines, and created unnecessary consing.  I'm convinced the current
>> algorithm in completion-pcm--hilit-commonality is fine for these
>> 1-char edge cases, given that the assumptions hold.
>
> I very much disagree with this

My assertion that your solution is complex is comparative, of course,
not absolute.  It is _more_ complex that some other existing solution --
in this case my three-line solution -- and I say that because of the
factual observation that it changes much more than three lines,
introduces new code paths, etc.  

For now, I believe the original problem that started this bug report,
which dealt with flex and substring completion, is fixed by my patch.
Your failed user experience of typing "R" to perform "M-x R" should now
be correct, as far as I can tell.

Of course, you say your current patch fixes _more_ things, but I've yet
to understand what those presumably broken things are.  Perhaps once all
of this is understood we will come to the conclusion that your solution
is indeed the simplest possible.  Or perhaps we won't.

> due to 1. the test cases

The test cases should be independent of the implementation, so adding
more tests doesn't make the implemented solution any simpler.  It does
make solutions _simpler to simplify_, which is why I welcome tests.

However, tests themselves should map to real-world problems (you know,
the ones that you describe in terms of Emacs -Q reproduction recipes).
And I'm still having trouble understanding exactly what these are for
some tests.  But I'm spending time to work on that.

> 2. the  previous reply to Stefan where I tried to explain the shortcomings of
> the current implementation of completion-pcm--hilit-commonality.

I don't understand these shortcomings, yet.  I think they should be
evidenced more clearly, in terms of actual user-experienceable problems.

> Also, could you point to the unnecessary consing?

completion-pcm--count-leading-holes and completion-pcm--match-size both
add consing.  That is clear to see from their respective
implementations.  I don't know if they can be made non-consing, though I
suspect they can.  Anyway, using them as they are adds a small amount
consing.

> I believe my patch is faster than the current implementation.

Why do you believe that?  Do you have any benchmarks to demonstrate it?

>> I'm not aware that it's not sound, but I do believe it's too complex and
>> not well understood.  In constrast, I can understand the three-line fix
>> I did earlier and which covers all of Darios's test cases for the flex
>> completion style.  Previously it was failing 7 cases, now it is only
>> failing these 3.
>
> Making the `any' explicit was also the very first thing I suggested, but
> further testing pointed to problems it doesn’t solve, which are indeed
> parts of the tests that are failing.

My latest proposal doesn't make the any explicit.  I'll try to
understand what the intention is behind the tests that are failing.

> I can add more tests if you think that would help.

Yes, it would.  But any test that you add should bring about evidence of
a real-world problem.  I.e., to state the obvious but make my point clear,
the assertion:

    (should (eq 'foo 'bar))
    
fails spectacularly but doesn't bring about such evidence.

João