bug#43598: replace-in-string: finishing touches

[Lars Ingebrigtsen]

Mattias Engdegård <mattiase@acm.org> writes:

> 1. Check the range of the START-POS argument so that we don't crash.
> The permitted range is [0..N] where N is (length HAYSTACK), thus we
> permit a start right after the last character but no further.
> We could also return nil in these cases but I think an error is more useful.

Good point.  :-)

> 2. Make the docs more precise about various things.
>
> 3. Slight simplification of the implementation logic to avoid testing
> the same conditions multiple times.
>
> 4. More tests, especially for edge cases. Can't have too many!

It all looks good to me; please apply.

> One test still fails:
>
>  (string-search "ø" "\303\270")
>
> which should return nil but currently matches.
> I think it's wrong to convert the needle to unibyte (using
> Fstring_as_unibyte) in this case, but I haven't decided what the best
> solution would be.

Yeah, that's the bit I was most unsure about, because it just didn't
look quite correct to me, but I couldn't come up with the correct test
case last night; thanks.

> We should also consider the optimisations:
> - If SCHARS(needle)>SCHARS(haystack) then no match is possible.

Yup.

> - If either needle or haystack is all-ASCII (all bytes in 0..127),
> then we can use memmem without conversion.

Right, so if the multibyteness differs, then do another check to see
whether both strings are all-ASCII anyway, and do the comparison without
conversion...  Yes, makes sense to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no