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

[Alexander Harlap]

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
>