Re: [PATCH] handle multibyte stores larger than char in strlen (PR 91183, 86888)

[Jeff Law <law at redhat dot com>]

On 7/22/19 5:17 PM, Martin Sebor wrote:

>> Umm "store_b4_nul"?  Are you really trying to save 3 characters in the
>> name by writing it this way?  :-)
> 
> I'm actually saving four characters over "store_before_nul" ;-)
:-)

> 
> It's just a name I picked because I like it.  Would you prefer to
> go back to the original 'cmp' instead?  It doesn't get much shorter
> than that, or less descriptive, especially when there is no comment
> explaining what it's for.  (FWIW, I renamed it because I found myself
> going back to the description of compare_nonzero_chars to remember
> what cmp actually meant.)
Fair enough.  Though "b4" reads like it belongs in a text message to me.
  I don't want to get nitty over this.  Will s/b4/before/ work for you?

If you wanted to add a comment before the variable, that would be good
as well, particularly since we don't have a good name.


> 
>> You've got two entries in the array, but the comment reads as if there's
>> just one element.  What is the difference between the two array elements?
> 
> Since handle_store deals with sequences of one or more bytes some
> of the optimizations it implements(*) need to consider both where
> the first byte of the sequence is stored and where the last one is.
> The first element of this array is for the first byte and the second
> one is for the last character.  The comment a few lines down is meant
> to make the distinction clear but we can expand the comment above
> the variable even more.
I think  my brain locked up with the "b4".  Maybe it went into a mode
where I'm trying to parse texts from my teenager which is clearly
incompatible with reading code. :-)

> 
>       /* The offset of the last stored byte.  */
>       unsigned HOST_WIDE_INT endoff = offset + lenrange[2] - 1;
> 
> (lenrange[2] is the size of the store.)
> 
> [*] E.g., the one we discussed not so long ago (PR90989) or the one
> that removes a store of '\0' over the terminating '\0'.
> 
>>
>> I didn't see anything terribly concerning so far, but I do want to look
>> at handle_store again after the comment is fixed so that I'm sure I'm
>> interpreting things correctly.
> 
> Does this help?
> 
>   /* The corresponding element is set to 1 if the first and last
>      element, respectively, of the sequence of characters being
>      written over the string described by SI ends before
>      the terminating nul (if it has one), to zero if the nul is
>      being overwritten but not beyond, or negative otherwise.  */
Yea.  I also note a /* */ empty comment in handle_store, you probably
wanted to write something there :-)

OK with the nit fixes noted earlier, variable name fix and comment fixes.


jeff