[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (S): 8143958: CDS Shared flags need constraint function
From:       Gerard Ziemski <gerard.ziemski () oracle ! com>
Date:       2016-03-29 18:50:20
Message-ID: 2F00E155-ED61-4064-933F-250869CAE13B () oracle ! com
[Download RAW message or body]

Thank you for the review!

> On Mar 29, 2016, at 12:07 PM, Jiangli Zhou <jiangli.zhou@oracle.com> wrote:
> 
> Hi Gerard,
> 
> Looks good.
> 
> Thanks,
> Jiangli
> 
> > On Mar 28, 2016, at 12:13 PM, Gerard Ziemski <gerard.ziemski@oracle.com> wrote:
> > 
> > hi Ioi,
> > 
> > Thank you for there review.
> > 
> > I have factored out the common code as you suggested and I like it now much \
> > better. 
> > Updated webrev at http://cr.openjdk.java.net/~gziemski/8143958_rev2
> > 
> > 
> > cheers
> > 
> > > > 
> > > On Mar 25, 2016, at 3:47 PM, Ioi Lam <ioi.lam@oracle.com> wrote:
> > > 
> > > Hi Gerard,
> > > 
> > > This looks OK. I have a general comment about \
> > > commandLineFlagConstraintsRuntime.cpp. There's a lot of boiler-plate code like \
> > > this: 
> > > 136   if (value > max) {
> > > 137     CommandLineError::print(verbose,
> > > 138                             "SharedReadWriteSize (" SIZE_FORMAT ") must be \
> > > " 139                             "smaller than (" SIZE_FORMAT ")\n",
> > > 140                             value, max);
> > > 141     return Flag::VIOLATES_CONSTRAINT;
> > > 142   } else {
> > > 143     return Flag::SUCCESS;
> > > 144   }
> > > 
> > > 
> > > Should that be wrapped in a helper function like this?
> > > 
> > > return CommandLineError::check_max(value, max, "SharedReadWriteSize");
> > > 
> > > Thanks
> > > - Ioi
> > > 
> > > On 3/25/16 9:26 AM, Gerard Ziemski wrote:
> > > > hi all,
> > > > 
> > > > Please review this small fix which adds constraints to CDS flags. Please note \
> > > > that the max range value is based on the sum of min values of the other 3 \
> > > > flags, so setting one flag's value to max while leaving the other 3 flags at \
> > > > their defaults will trigger constraint failure, which \
> > > > test/runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java \
> > > > accounts for. 
> > > > https://bugs.openjdk.java.net/browse/JDK-8143958
> > > > http://cr.openjdk.java.net/~gziemski/8143958_rev1
> > > > 
> > > > Passes JPRT hotspot and RBT \
> > > > CommandLine/OptionsValidation/TestOptionsWithRanges on all platforms. 
> > > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]