RFR(XS) 8170981: Possible access to char array with negative index

[Schmidt, Lutz]

Hi David, 

thanks for the review. Please find my responses below, next to your
comments. 

About your question “What’s that code doing after all?”: I can only try to
understand and describe what the original author tried to accomplish. Here
we go:
 
Flag names are assumed (better: hoped) to have a length of at most 40
characters. The “=“ sign is aligned according to that assumption. The
author obviously did not want to move the ‘=‘ further to the right to
accommodate for just the few flags with names longer than 40 characters.
On the other hand, the author wanted to keep the alignment of subsequent
columns intact. So he introduced an elastic buffer (10 spaces) which is
printed after the flag value. For each flag name character in excess of
40, one space is trimmed from that elastic buffer.

There is a new iteration (just the comment and alignment adjustments) of
the webrev:
    http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981.v2/


Regards,
Lutz



On 12.12.16, 23:19, "David Holmes" <david.holmes at oracle.com> wrote:

>Hi Lutz,
>
>On 13/12/2016 3:10 AM, Schmidt, Lutz wrote:
>> Rachel,
>>
>>
>> thanks a lot for the review. An updated webrev (according to my comments
>> below) can be found here:
>>    http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981.v1
>
>I have a few nits with this:
>
>  476     // nSpaces spaces below to adjust the space between the flag
>value and the
>
>The array is still hardwired to 10 spaces so the comment should still
>say 10 IMHO - otherwise it sounds like the number of space is
>dynamically determined.

OK. Concern understood. Comment changed back to 10.

>
>Please add a trailing comment here:
>
>480     char spaces[] = "          ";  // 10 spaces plus NUL

OK. Comment added.

>
>Style nit:
>
>481     const unsigned int maxFlagLen = 50;
>482     const unsigned int nSpaces    = strlen(spaces);
>
>no reason to align the =.

My reason: personal aesthetics. OpenJDK code is not uniform in that
respect. Alignment removed.

>
>484     st->print("%9s %-*s = ", _type, maxFlagLen-nSpaces, _name);
>
>I'm not quite sure exactly how this code tries to print different length
>flag names. The above will left-justify and space-pad to at least 40
>characters IIUC. If strlen(_name) is < 40 then we later add more padding
>(up to 10) from the spaces array. Why do we need to do that? Why don't
>we simply use %-50s in the first place ??

See explanation above. Padding supposed to happen behind the flag value.

>
>Thanks,
>David
>
>> Regards,
>> Lutz
>>
>>
>> On 12.12.16, 17:07, "hotspot-runtime-dev on behalf of Rachel Protacio"
>> <hotspot-runtime-dev-bounces at openjdk.java.net on behalf of
>> rachel.protacio at oracle.com> wrote:
>>
>>> Hi,
>>>
>>> I wonder whether it's better to declare
>>>
>>>    const char* spaces = "         ";
>>>
>>> and then set nSpaces as strlen(spaces), so it's clear that the spaces
>>> string will always be consistent with nSpaces?
>>
>> Good idea, Rachel! I feel uncomfortable with the ³disconnected²
>> declarations as well. I had to use a non-constant char array because a
>> Œ\0¹ is stored into it.
>>
>> Btw, a ³beautiful² fix would abandon the spaces array altogether and
>> implement columnar alignment with the help of tty->fill_to(pos). But
>>that
>> would be a reengineering effort which might not take the Java9 hurdle.
>>
>>>
>>> Otherwise, looks good to me.
>>> Rachel
>>>
>>> On 12/12/2016 9:45 AM, Schmidt, Lutz wrote:
>>>> Hi,
>>>>
>>>> may I please ask for reviews and sponsorship for this small fix in
>>>> command
>>>> line flag printing?
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170981
>>>> webrev: http://cr.openjdk.java.net/~simonis/webrevs/2016/8170981
>>>>
>>>> Thank you!
>>>> Lutz
>>>>
>>>
>>