@Param support for enum

[Aleksey Shipilev]

Thanks, the param-enum patch looks good now, and it seems to pass the
tests. Are you saying your recent param-enum patch broke
IterationBlackhole2Test?

-Aleksey

On 06/10/2014 01:16 AM, Joe Kearney wrote:
> The bugfix broke IterationBlackhole2Test, I'll have a quick look before
> committing that.
> 
> 
> On 9 June 2014 22:03, Joe Kearney <mail at joekearney.co.uk
> <mailto:mail at joekearney.co.uk>> wrote:
> 
>     New patch attached for enum params.
> 
> 
>     On 9 June 2014 21:36, Joe Kearney <mail at joekearney.co.uk
>     <mailto:mail at joekearney.co.uk>> wrote:
> 
>         Static methods in BenchmarkGenerator - I didn't move them
>         because they were only used locally. Happy to move them out if
>         you'd prefer.
> 
>         Fair point on predicate length. I'm inclined to pull
>         isEmptyEnumParam(FieldInfo, String[]) into BenchmarkGenerator as
>         there are already a couple of uses of that. I considered an
>         inner if, but the else branch of that would be the same as the
>         non-enum case, from what I could see.
> 
>         Patch to follow, thanks for feedback.
> 
> 
>         On 9 June 2014 21:14, Aleksey Shipilev
>         <aleksey.shipilev at oracle.com
>         <mailto:aleksey.shipilev at oracle.com>> wrote:
> 
>             Hi Joe,
> 
>             On 06/10/2014 12:02 AM, Joe Kearney wrote:
>             > Aleksey, as discussed here is the patch. It includes the
>             main change in
>             > BenchmarkGenerator and StateObjectHandler, plus runtime
>             and compile-time
>             > tests.
> 
>             Thanks!
> 
>             > Couple of notes:
>             > * I factored out addParameterValuesToGroup because the
>             code was
>             > duplicated between step 1 and 2 of @Param discovery.
> 
>             Looks good, but please move both static methods into
>             BenchmarkUtils.
> 
>             > * I'd advocate renaming getEnumConstants to
>             getEnumConstantNames, but
>             > maybe that's just a matter of taste.
> 
>             Truly, a matter of taste :)
> 
>             > * I'm pretty sure there's a typo-bug in BenchmarkGenerator
>             line 311, it
>             > should loop over fields from state#getFields, not over
>             fields in clazz,
>             > an unused local variable warning.
> 
>             D'oh! Thanks for catching it, can you submit a separate patch?
> 
>             > Please let me know any feedback you have. I'm around
>             tonight for any
>             > changes you'd like to make.
> 
>             Okay, here it goes:
> 
>              * Static utility methods better be in BenchmarkUtils.
>              * Please avoid long predicates: I understand it is easier
>             to write out
>             " && !fi.getType().isEnum()", but I think these checks deserve a
>             separate inner if.
>              * ...this, AFAIU, will save you from additional condition in
>             "isParamValueConforming": the control should not go there
>             for blank
>             arguments
> 
>             Otherwise looks good. I'll wait for your new patch.
> 
>             Thanks,
>             -Aleksey.
> 
> 
> 
>