Corrected: RFR 8059557 (XL): Validate JVM Command-Line Flag Arguments

[Alexander Harlap]

Hi Gerald,

Unnamed parameter is standard C++.
No restrictions at all.

Alex

On 5/21/2015 12:54 PM, Gerard Ziemski wrote:
> hi Alexander,
>
> Yes, handling it this way was an alternative, but I thought it 
> required a C++ compiler with some specific feature support?  C++ 11?
>
> It was my understanding that such solution was more restrictive, than 
> the one I used.
>
> Thank you.
>
> On 5/21/2015 11:26 AM, Alexander Harlap wrote:
>> And what about this:
>>
>> void emit_range_bool(const char* /* name */)            { }
>>
>> Alex
>>
>> On 5/21/2015 12:09 PM, Gerard Ziemski wrote:
>>> hi David,
>>>
>>> Thank you for taking the time to review this considerable change. I 
>>> have responded to your points below:
>>>
>>>
>>> On 5/18/2015 12:28 AM, David Holmes wrote:
>>>>
>>>> A few comments:
>>>>
>>>> For constructs like:
>>>>
>>>> void emit_range_bool(const char* name)            { (void)name; /* 
>>>> NOP */ }
>>>>
>>>> I seem to recall that this was not sufficient to avoid "unused" 
>>>> warnings with some compilers - which is presumably why you did it?
>>>
>>> That's indeed the reason why I did. A search on internet reveals 
>>> that this seems to be the preferred way of handling such case, and 
>>> it works for all the currently building JPRT platforms. Is that 
>>> enough? If there is a question of whether this works on some unusual 
>>> compiler, can we leave it up to the the engineers dealing with that 
>>> compiler, which certainly would make them more of experts in such 
>>> situation than me? If not, how would you recommend we handle this 
>>> differently now?
>>>
>>>
>>>>
>>>> ---
>>>>
>>>> In src/share/vm/runtime/globals.hpp it says:
>>>>
>>>>   see "checkRanges" function in arguments.cpp
>>>>
>>>> but there is no such function in arguments.cpp
>>>
>>> Done.
>>>
>>>>
>>>> ---
>>>>
>>>> src/share/vm/runtime/arguments.cpp
>>>>
>>>> The set_object_alignment() functions seems to have some redundant 
>>>> constraint assertions. As does verify_object_alignment() - seems to 
>>>> me that everything in verify_object_alignment should either be in 
>>>> the constraint function for ObjectAlignmentInBytes or one for 
>>>> SurvivorAlignmentInBytes - though the combination of verification 
>>>> and the actual setting of SurvivorAlignmentInBytes may be a problem 
>>>> in the new architecture. If you can't get rid of 
>>>> verify_object_alignment() I'd be tempted to not process it the new 
>>>> way at all, as splitting the constraint checking just leads to 
>>>> confusion IMO.
>>>
>>> I moved the code to constraints and in fact this little bit of 
>>> refactoring looks very nice indeed.
>>>
>>>
>>>>
>>>> The changes to test/runtime/contended/Options.java suggest to me 
>>>> that a constraint is missing on ContendedPaddingWidth - that it is 
>>>> a multiple of 8 (BytesPerLong). That is (still) checked in 
>>>> arguments.cpp (and given it is still checked there is no need to 
>>>> have removed it from the test).
>>>
>>> I could argue whether the tests is really loosing a value in 
>>> displaying only one error at the time, but I tried to modify my 
>>> feature, so that it could also detect and report more than one 
>>> violation at the time, and it turned out as doable in these cases, 
>>> so this is now resolved, with no perceived functionality regression.
>>>
>>>
>>>>
>>>> ---
>>>>
>>>> Some of the test changes, such as:
>>>>
>>>> test/gc/g1/TestStringDeduplicationTools.java
>>>>
>>>> seem to be losing some of what they test. Not only is the test 
>>>> checking the value is detected as erroneous, but it also detects 
>>>> that the user is told in what way it is erroneous. The updated test 
>>>> doesn't validate that part of the argument processing logic
>>>
>>> Done, with same fix as above.
>>>
>>>
>>>>
>>>> ----
>>>>
>>>> There is some inconsistency in the test changes, sometimes you use:
>>>>
>>>>   shouldContain("outside the allowed range")
>>>>
>>>> and sometimes:
>>>>
>>>>   shouldContain("is outside the allowed range")
>>>
>>> Done.
>>>
>>>
>>> cheers
>>>
>>
>>
>>
>