@Param support for enum

[Joe Kearney]

Sorry that wasn't clear - no the typo-fix seemed to break a test, but I
can't reproduce it now. I think Eclipse did something funny with the
compiled classes while maven was running.

All tests pass now, patch for the state field annotation validation typo
bug is attached.


On 9 June 2014 22:27, Aleksey Shipilev <aleksey.shipilev at oracle.com> wrote:

> 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.
> >
> >
> >
> >
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: param-for-bg-typo.patch
Type: text/x-patch
Size: 915 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/jmh-dev/attachments/20140609/b4e17de2/param-for-bg-typo.patch>