bug#40000: 27.0.60; next-single-char-property-change hangs on bad argument

[Federico Tedin]

Hey Eli, thanks for the detailed feedback.

> What you used as the description of the change is actually the
> explanation why the change was made.  The description of the change
> would be something like this:
>
>   * src/textprop.c (Fnext_single_char_property_change): Clarify in
>   the doc string the behavior when LIMIT is past the end of OBJECT.
>   Stop the search when position gets to end of buffer, for when LIMIT
>   is beyond that.  (Bug#40000)
>
> The explanation should ideally be in the comments to the code.  In
> this case, I'm not even sure we need an explanation, since there's a
> reference to the bug report, and the explanation and the fix are quite
> simple.

Aah OK, that makes sense. I agree that the bug number should be enough
if someone wants to find an explanation for the changes; I've changed
the commit message to the one you proposed.

>> -In a string, scan runs to the end of the string, unless LIMIT is non-nil.
>> -In a buffer, if LIMIT is nil or omitted, it runs to (point-max), and the
>> -value cannot exceed that.
>> -If the optional fourth argument LIMIT is non-nil, don't search
>> -past position LIMIT; return LIMIT if nothing is found before LIMIT.
>> +In a string, scan runs to the end of the string, unless LIMIT is
>> +non-nil, in which case its value is returned if nothing is found
>> +before it.
>> 
>> -The property values are compared with `eq'.
>> -If the property is constant all the way to the end of OBJECT, return the
>> -last valid position in OBJECT.  */)
>> +In a buffer, if LIMIT is nil or omitted, it runs to (point-max).  If
>> +LIMIT is non-nil, scan does not go past it, and the smallest of
>> +(point-max) and LIMIT is returned.
>> +
>> +The property values are compared with `eq'.  */)
>
> Try to avoid passive tense in documentation, it makes the text longer
> and sometimes harder to understand.  Here's how I'd rephrase this:
>
>   In a string, scan runs to the end of the string, unless LIMIT is non-nil.
>   In a buffer, scan runs to end of buffer, unless LIMIT is non-nil.
>   If the optional fourth argument LIMIT is non-nil, don't search
>   past position LIMIT; return LIMIT if nothing is found before LIMIT.
>   However, if OBJECT is a buffer and LIMIT is beyond the end of the
>   buffer, this function returns `point-max', not LIMIT.

Good point, for some reason I am biased towards using the passive
tense. I've changed the documentation string.

>   The property values are compared with `eq'.
>
>> +        if (XFIXNAT (position) >= ZV)
>> +          {
>> +            XSETFASTINT (position, ZV);
>> +            break;
>> +          }
>
> Here we expect 'position' to have the value of ZV already, right.
> Then there's no need to use XSETFASTINT; if you want to make sure we
> never get here with value >= ZV, you can add an assertion.

Aah right, because `next-char-property-change' never returns
values larger than `point-max'. So in the last iteration, `position'
will be `ZV'. I don't feel like it's necessary to add an assertion
though, unless the code inside the loop somehow was modified to be
longer/more complex. I'm attaching an updated patch.

- Fede

0001-Prevent-hanging-in-next-single-char-property-change.patch
Description: Text Data