GNU bug report logs -
#40000
27.0.60; next-single-char-property-change hangs on bad argument
Previous Next
Reported by: Yuan Fu <casouri <at> gmail.com>
Date: Mon, 9 Mar 2020 15:41:02 UTC
Severity: normal
Found in version 27.0.60
Done: Eli Zaretskii <eliz <at> gnu.org>
Bug is archived. No further changes may be made.
Full log
View this message in rfc822 format
[Message part 1 (text/plain, inline)]
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 (text/x-diff, attachment)]
This bug report was last modified 5 years and 9 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.