Thanks for the review Joe, I added a space and pushed. cheers /Joel
> On 22 aug 2015, at 01:12, Joseph D. Darcy <joe.da...@oracle.com> wrote: > > Belatedly, the > > http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ > > webrev looks fine other than a missing space in a line of > AnnotationSuport.java: > > 208 }catch (Throwable t) { // from > InvocationHandler::invoke > > Thanks, > > -Joe > > On 3/28/2015 3:24 AM, Joel Borggrén-Franck wrote: >> Hi, >> >>> On 27 Feb 2015, at 11:06, Peter Levart <peter.lev...@gmail.com> wrote: >>> On 02/27/2015 10:27 AM, Joel Borggrén-Franck wrote: >>>>> On 26 feb 2015, at 22:35, Peter Levart <peter.lev...@gmail.com> >>>>> wrote: >>>>> On 02/26/2015 10:27 PM, Peter Levart wrote: >>>>> >>>>>> The m.setAccessible(true) for the methods is needed to access methods of >>>>>> non-public annotations, right? This call could be moved to >>>>>> AnnotationType constructor as there it will be performed only once per >>>>>> Method object. >>>>>> >>>>> ...which will have the added benefit in that it will guarantee that only >>>>> one MethodAccessor object per Method will ever be constructed instead of >>>>> two… >>>>> >>>>> >>>> I don’t see this. setAccessible sets override in AccessibleObject, I don’t >>>> see a new MethodAccessor being generated here. >>> My fault! I was mistakenly thinking of Field objects in the context of >>> setAccessible(boolean). If you use a Field object in two modes it will >>> create and retain two different FieldAccessors (because they are different >>> implementations in case field is final). >>> >>>> But I agree with you, and setting it as accessible in the AnnotationType >>>> constructor should arguably be more secure since then we know it isn’t >>>> shared since we just got our copy fresh from getDeclaredMethods(). >>> That's a good point from maintainability perspective, yes, if someone some >>> time decides to "optimize" the AnnotationType. I think it would be nice if >>> AnnotationType documents that members() method returns Method objects that >>> are pre-conditioned with setAccessible(true) and that users should not >>> change this flag. >>> >> I don’t want to do this in AnnotationType without a bigger overhaul that may >> be slightly incompatible and therefor should be 9 only. I do want to >> backport this fix to 8 however, so here is an alternative solution that >> should be safe and correct at the cost of extra allocation in the path for >> custom implementations of annotations (that should be rare). >> >> New webrev: >> >> http://cr.openjdk.java.net/~jfranck/8073056/webrev.02/ >> >> cheers >> /Joel >
