On 7/24/19 11:06 AM, Jeff Law wrote:
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 it's distracting I'll change it.
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. :-)
That's a good enough argument to change it :)
/* 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 :-)
I did initially want to write something there but then I wasn't
sure the conditional was quite right. I went to investigate and
forgot to fix the conditional. There was an unnecessarily test
there so I removed both it and the comment placeholder.
OK with the nit fixes noted earlier, variable name fix and comment fixes.
Committed in r273783 after retesting and including a test for
the warning that I had left out of the patch I posted here.
Martin
PS I suspect some of the tests I added might need tweaking on
big-endian systems. I'll deal with them tomorrow.
