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
