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

[Thomas Stüfe]

On Mon, Dec 12, 2016 at 11:19 PM, 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.
>
> Please add a trailing comment here:
>
> 480     char spaces[] = "          ";  // 10 spaces plus NUL
>
> Style nit:
>
> 481     const unsigned int maxFlagLen = 50;
> 482     const unsigned int nSpaces    = strlen(spaces);
>
> no reason to align the =.
>
> 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 ??
>
>
Also, could we not just use outputStream::fill_to() instead of putting
those spaces together manually?



> 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
>>>>
>>>>
>>>
>>