JDK 9 request for specification changes of JDK-8032230: Enhance javax.a.p.RoundEnvironment after repeating annotations
joe darcy
joe.darcy at oracle.com
Thu May 26 05:26:43 UTC 2016
Hi Jon,
Will push with an explanatory note on the TestingRoundEnvironment class.
Thanks,
-Joe
On 5/25/2016 7:58 PM, Jonathan Gibbons wrote:
> Well, it did seem strange to have the overriding method (just) call
> the method it was overriding ;-)
>
>
> One alternative for your consideration:
>
> Instead of
>
> 109 @Override
> 110 public Set<? extends Element>
> getElementsAnnotatedWithAny(TypeElement... a) {
> 111 // Default method defined in the interface
> 112 return
> RoundEnvironment.super.getElementsAnnotatedWithAny(a);
> 113 }
>
> how about
>
> 109 // inherit default impl of this method from the interface
> 110 // public Set<? extends Element>
> getElementsAnnotatedWithAny(TypeElement... a)
>
>
> Or, you could put comments on the class itself.
> // This class delegates all abstract methods on the interface to
> the {@code RoundEnvironment}
> // provided when the class is created, relying on the default
> methods provided in the interface
> // for ....(fill in details)...
>
> Whichever way you go, overriding the methods or not, I think some
> clarifying comments are
> in order.
>
> -- Jon
>
>
> On 05/25/2016 07:46 PM, joe darcy wrote:
>> Hi Jon,
>>
>>
>> On 5/25/2016 5:57 PM, Jonathan Gibbons wrote:
>>> Joe,
>>>
>>> TestingRoundEnvironment is (mostly) cleaver, but why do you bother
>>> to override the default methods, and then call them via super?
>>>
>>> Why not just not override them in the first place? Since this is a
>>> class that is simply implementing an interface, the only method can
>>> can be invoked is the default method from the interface.
>>
>> I thought, perhaps incorrectly, that explicitly calling the interface
>> methods (as opposed to not overriding those methods) would more
>> clearly indicate the intention of the wrapper class.
>>
>> Do you think a more explicit comment about this intention on the
>> wrapper class would help clarify matters?
>>
>> -Joe
>>
>>>
>>> -- Jon
>>>
>>>
>>>
>>> On 05/24/2016 09:18 AM, joe darcy wrote:
>>>> Updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~darcy/8032230.1/
>>>>
>>>> Now with specialized implementation in javac's implementation of
>>>> RoundEnvironment and separate tests of the default methods in the
>>>> interface and the javac implementation.
>>>>
>>>> Thanks,
>>>>
>>>> -Joe
>>>>
>>>>
>>>> On 5/23/2016 11:21 AM, joe darcy wrote:
>>>>> Hello,
>>>>>
>>>>> Please now review a webrev including the specification and some
>>>>> tests:
>>>>>
>>>>> http://cr.openjdk.java.net/~darcy/8032230.0/
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Joe
>>>>>
>>>>>
>>>>> On 5/18/2016 2:02 PM, joe darcy wrote:
>>>>>> Combining suggestions, two methods
>>>>>>
>>>>>> default Set<? extends Element>
>>>>>> getElementsAnnotatedWithAny(TypeElement... annotations)
>>>>>>
>>>>>> default Set<? extends Element>
>>>>>> getElementsAnnotatedWithAny(Set<Class<? extends Annotation>>
>>>>>> annotations)
>>>>>>
>>>>>> No varargs in the second case because of heap pollution with the
>>>>>> wildcarded Class type and the inability to use @SafeVarargs on
>>>>>> this method since it can be overridden.
>>>>>>
>>>>>> I'd prefer a set of methods like the above that weren't only tied
>>>>>> to repeating annotations since I think there are other use cases
>>>>>> where it is helpful to find more than one annotation at a time.
>>>>>>
>>>>>> Fuller diff below.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Joe
>>>>>>
>>>>>>
>>>>>> /**
>>>>>> + * Returns the elements annotated with one or more of the given
>>>>>> + * annotation types.
>>>>>> + *
>>>>>> + * @apiNote This method may be useful when processing repeating
>>>>>> + * annotations by looking for an annotation type and its
>>>>>> + * containing annotation type at the same time.
>>>>>> + *
>>>>>> + * @implSpec The default implementation of this method
>>>>>> creates an
>>>>>> + * empty result set, iterates over the annotations in the
>>>>>> argument
>>>>>> + * set calling {@link
>>>>>> #getElementsAnnotatedWith(TypeElement)} on
>>>>>> + * each annotation and adding those results to the result
>>>>>> + * set. Finally, the contents of the result set are returned
>>>>>> as an
>>>>>> + * unmodifiable set.
>>>>>> + *
>>>>>> + * @param annotations annotation types being requested
>>>>>> + * @return the elements annotated with one or more of the given
>>>>>> + * annotation types, or an empty set if there are none
>>>>>> + * @throws IllegalArgumentException if the any elements of the
>>>>>> + * argument set do not represent an annotation type
>>>>>> + * @since 9
>>>>>> + */
>>>>>> + default Set<? extends Element>
>>>>>> getElementsAnnotatedWithAny(TypeElement... annotations){
>>>>>> + HashSet<Element> result = new HashSet<>();
>>>>>> + for (TypeElement annotation : annotations) {
>>>>>> + result.addAll(getElementsAnnotatedWith(annotation));
>>>>>> + }
>>>>>> + return Collections.unmodifiableSet(result);
>>>>>> + }
>>>>>>
>>>>>> + /**
>>>>>> + * Returns the elements annotated with one or more of the given
>>>>>> + * annotation types.
>>>>>> + *
>>>>>> + * @apiNote This method may be useful when processing repeating
>>>>>> + * annotations by looking for an annotation type and its
>>>>>> + * containing annotation type at the same time.
>>>>>> + *
>>>>>> + * @implSpec The default implementation of this method
>>>>>> creates an
>>>>>> + * empty result set, iterates over the annotations in the
>>>>>> argument
>>>>>> + * set calling {@link #getElementsAnnotatedWith(Class)} on
>>>>>> + * each annotation and adding those results to the result
>>>>>> + * set. Finally, the contents of the result set are returned
>>>>>> as an
>>>>>> + * unmodifiable set.
>>>>>> + *
>>>>>> + * @param annotations annotation types being requested
>>>>>> + * @return the elements annotated with one or more of the given
>>>>>> + * annotation types, or an empty set if there are none
>>>>>> + * @throws IllegalArgumentException if the any elements of the
>>>>>> + * argument set do not represent an annotation type
>>>>>> + * @since 9
>>>>>> + */
>>>>>> + default Set<? extends Element>
>>>>>> getElementsAnnotatedWithAny(Set<Class<? extends Annotation>>
>>>>>> annotations){
>>>>>> + HashSet<Element> result = new HashSet<>();
>>>>>> + for (Class<? extends Annotation> annotation :
>>>>>> annotations) {
>>>>>> + result.addAll(getElementsAnnotatedWith(annotation));
>>>>>> + }
>>>>>> + return Collections.unmodifiableSet(result);
>>>>>> + }
>>>>>>
>>>>>>
>>>>>> On 5/18/2016 1:55 PM, Jonathan Gibbons wrote:
>>>>>>> Jan,
>>>>>>>
>>>>>>> You're getting very close to suggesting a method that takes a
>>>>>>> predicate, and we could provide a variety of predicates for
>>>>>>> (anyOf, allOf) x (Annotation, AnnotationMirror) x (varags,
>>>>>>> collection)
>>>>>>>
>>>>>>> -- Jon
>>>>>>>
>>>>>>> On 05/18/2016 01:48 PM, Jan Lahoda wrote:
>>>>>>>> On 18.5.2016 22:26, joe darcy wrote:
>>>>>>>>> Hi Jon,
>>>>>>>>>
>>>>>>>>> On 5/18/2016 1:05 PM, Jonathan Gibbons wrote:
>>>>>>>>>> It's sad to see the preference given to the more intellectually
>>>>>>>>>> suspect of the two possibilities.
>>>>>>>>>
>>>>>>>>> Agreed, sad but pragmatic.
>>>>>>>>
>>>>>>>> Would it make sense to use varargs instead of a Set? If we
>>>>>>>> assume the typical use will be something like:
>>>>>>>> round.getElementsAnnotatedWithAny(annotation, containerAnnotation)
>>>>>>>>
>>>>>>>> it might be more convenient to have the method take a vararg
>>>>>>>> than a Set (and would avoid the problem with the erasure clash
>>>>>>>> as another benefit).
>>>>>>>>
>>>>>>>> Jan
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> It would be nice to see the nice name given to the
>>>>>>>>>> intellectually
>>>>>>>>>> superior of the possibilities (i.e AnnotationMirror) and then
>>>>>>>>>> figure
>>>>>>>>>> out how to deal with the other case.
>>>>>>>>>
>>>>>>>>> How about the name "getElementsAnnotatedWithAny" for both
>>>>>>>>> variations?
>>>>>>>>> That potentially avoids confusion over whether or not the
>>>>>>>>> elements have
>>>>>>>>> to be modified with any of the annotations or all of them.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> As well as the possibility of another method name, have you ever
>>>>>>>>>> considered the possibility of conversion functions of
>>>>>>>>>> Elements/Types/<something-new> that can convert between
>>>>>>>>>> (collection
>>>>>>>>>> of) Annotation and (collection of) AnnotationMirror?
>>>>>>>>>
>>>>>>>>> Internally, for the existing methods javac does convert the
>>>>>>>>> Class-based
>>>>>>>>> version to the TypeElement based version, but I don't think we
>>>>>>>>> want the
>>>>>>>>> specification to require that.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> -Joe
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> -- Jon
>>>>>>>>>>
>>>>>>>>>> On 05/18/2016 12:55 PM, joe darcy wrote:
>>>>>>>>>>> Hello,
>>>>>>>>>>>
>>>>>>>>>>> Please review the patch below which proposes a new method to
>>>>>>>>>>> address
>>>>>>>>>>>
>>>>>>>>>>> JDK-8032230: Enhance javax.a.p.RoundEnvironment after
>>>>>>>>>>> repeating
>>>>>>>>>>> annotations
>>>>>>>>>>>
>>>>>>>>>>> At present, this is just a review of the specification of
>>>>>>>>>>> the default
>>>>>>>>>>> method in the interface and *not* a more optimized
>>>>>>>>>>> implementation in
>>>>>>>>>>> the javac RoundEnvironemnt implementation (and not any tests
>>>>>>>>>>> that
>>>>>>>>>>> would be needed for the new functionality).
>>>>>>>>>>>
>>>>>>>>>>> Note that the bug proposes adding two methods
>>>>>>>>>>>
>>>>>>>>>>> RoundEnvironment.getElementsAnnotatedWith(Set<Class<? extends
>>>>>>>>>>> Annotation>> s)
>>>>>>>>>>> RoundEnvironment.getElementsAnnotatedWith(Set<AnnotationMirror>
>>>>>>>>>>> s)
>>>>>>>>>>>
>>>>>>>>>>> but these methods would clash since their erasure is the
>>>>>>>>>>> same. *sad
>>>>>>>>>>> tromphone*
>>>>>>>>>>>
>>>>>>>>>>> Therefore, I'm only proposing to add the Class-based variant
>>>>>>>>>>> since
>>>>>>>>>>> that one is the more commonly used of the two.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> -Joe
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> + /**
>>>>>>>>>>> + * Returns the elements annotated with any of the given
>>>>>>>>>>> annotation
>>>>>>>>>>> + * types.
>>>>>>>>>>> + *
>>>>>>>>>>> + * @apiNote This method may be useful when processing
>>>>>>>>>>> repeating
>>>>>>>>>>> + * annotations by looking for an annotation type and its
>>>>>>>>>>> + * containing annotation type at the same time.
>>>>>>>>>>> + *
>>>>>>>>>>> + * @implSpec The default implementation of this method
>>>>>>>>>>> creates an
>>>>>>>>>>> + * empty result set, iterates over the annotations in
>>>>>>>>>>> the argument
>>>>>>>>>>> + * set calling {@link
>>>>>>>>>>> #getElementsAnnotatedWith(TypeElement)} on
>>>>>>>>>>> + * each annotation and adding those results to the result
>>>>>>>>>>> + * set. Finally, the contents of the result set are
>>>>>>>>>>> returned as an
>>>>>>>>>>> + * unmodifiable set.
>>>>>>>>>>> + *
>>>>>>>>>>> + * @param annotations annotation type being requested
>>>>>>>>>>> + * @return the elements annotated with the given
>>>>>>>>>>> annotation types,
>>>>>>>>>>> + * or an empty set if there are none
>>>>>>>>>>> + * @throws IllegalArgumentException if the any elements
>>>>>>>>>>> of the
>>>>>>>>>>> + * argument set do not represent an annotation type
>>>>>>>>>>> + * @since 9
>>>>>>>>>>> + */
>>>>>>>>>>> + default Set<? extends Element>
>>>>>>>>>>> getElementsAnnotatedWith(Set<Class<? extends Annotation>>
>>>>>>>>>>> annotations){
>>>>>>>>>>> + HashSet<Element> result = new HashSet<>();
>>>>>>>>>>> + for (Class<? extends Annotation> annotation :
>>>>>>>>>>> annotations) {
>>>>>>>>>>> + result.addAll(getElementsAnnotatedWith(annotation));
>>>>>>>>>>> + }
>>>>>>>>>>> + return Collections.unmodifiableSet(result);
>>>>>>>>>>> + }
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the compiler-dev
mailing list