RFR 8130459(M): Add additional validation after heap creation
Kim Barrett
kim.barrett at oracle.com
Thu Jul 23 17:20:12 UTC 2015
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. 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. 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?
More information about the hotspot-dev
mailing list