RFR 8130459(M): Add additional validation after heap creation

[sangheon.kim]

>> Arguments::do_after_ergo (not wedded to this name)
>>    // --- maybe check_ranges call goes here?
>>    calls CommandLineFlagConstraintList::check_constraints with
>>      CommandLineFlagConstraint::AfterErgo
>>    calls post_after_ergo_constraint_check
>>
>> Or maybe that should just be at the end of apply_ergo?
>> Let me apply first suggestion. 
>
> Let me apply first suggestion. 
Sorry Kim, let me cancel my previous answer.
If we directly call CommandLineFlagConstraintList::check_constraints, we 
don't have layering problem.

i.e. from Threads::create_vm().
   Arguments::apply_ergo();
   CommandLineFlagRangeList::check_ranges()
CommandLineFlagConstraintList::check_constraints(CommandLineFlagConstraint::AfterErgo);
   Arguments::post_after_ergo_constraint_check(constraint_result);

How about this?

Thanks,
Sangheon


On 07/23/2015 11:13 AM, sangheon.kim wrote:
> Hi Kim,
>
> On 07/23/2015 10:20 AM, Kim Barrett wrote:
>> On Jul 23, 2015, at 1:50 AM, sangheon.kim <sangheon.kim at oracle.com> 
>> wrote:
>>> As I thought CommandLineFlags is a main class for command line 
>>> flags, I wanted to concentrate all functionality there.
>>> However separating check functionality to CommandLineFlagRangeList 
>>> and CommandLineFlagConstraintList also seems reasonable.
>>> So as you suggested I will move into CommandLineFlagRangeList and 
>>> CommandLineFlagConstraintList.
>>> And then we can eliminate some functions from CommandLineFlags class.
>>>
>>>> Eliminating the check_constraints_of_after_xxx functions would also
>>>> involve moving the call to post_after_ergo_constraint_check, but I
>>>> think that's misplaced inside
>>>> CommandLineFlags::check_constraints_of_after_ergo.
>>> I will change to call post_after_ergo_constraint_check directly 
>>> since CommandLineFlags will not have constraint/range related 
>>> functions.
>>> But I don't think it is misplaced as CommandLineFlags managed all 
>>> constraint/range check functions, adding post work function there 
>>> seemed natural.
>>> If we need 'AfterMemoryInit' post work, I was planing to add in 
>>> CommandLineFlags::check_constraints_of_after_memory_init(). :)
>>>
>>>> Eliminating those check_constraints_of_after_xxx functions would have
>>>> the additional benefit of removing any concerns about their names; I
>>>> think another reviewer (David?) commented on them, and they strike me
>>>> as rather strange.
>>> I will change them.
>>> But again I wanted to centralize range/constraint related functions 
>>> at CommandLineFlags class. :-)
>> I agree that CommandLineFlags is the main class for command line
>> flags, but that doesn't mean all related functionality must be
>> directly implemented by that class.
> Yes, right.
> What I meant was if possible I wanted to have all at CommandLineFlags.
> But considering other things such as naming matters changed my mind to 
> go your suggestion.
> I also thought about this matter when I revised these parts. But 
> couldn't find clear reason for separating.
>
>>    Indeed, there are these related
>> CommandLineFlag{Range,Contraint}[List] classes that provide some of
>> the implementation of CommandLineFlags functionality.  And I expected
>> to find the implementation of range and constraint checking with the
>> corresponding classes, rather than with CommandLineFlags.
>>
>> Related to this, and specifically with the
>> post_after_ergo_constraint_check function, I think Arguments is a
>> layer that is built on top of CommandLineFlag and related classes
>> (among others), and having something in the latter directly calling
>> back up to Arguments is a layering breakage.
> Yes, I agree regarding layering things.
>
>>   I'd be fine with
>> something like the following instead:
>>
>> Arguments::do_after_ergo (not wedded to this name)
>>    // --- maybe check_ranges call goes here?
>>    calls CommandLineFlagConstraintList::check_constraints with
>>      CommandLineFlagConstraint::AfterErgo
>>    calls post_after_ergo_constraint_check
>>
>> Or maybe that should just be at the end of apply_ergo?
> Let me apply first suggestion.
>
> Thanks,
> Sangheon
>
>
>