Hi Joe, all,
the logic looks good to me.
In the tests I'm wondering whether to include an annotated wildcard
bound. There is:
307 public @AnnotType(11) Set<@AnnotType(13) ? extends Number>
fooNumberSet2() {return null;}
but nothing like
Set<? extends @AnnotType(13) Number> fooNumberSet2() {return null;}
I wouldn't expect problems for this, but a test would exercise some of
the code that gets added.
Best,
cu, WMD.
On Wed, Oct 10, 2018 at 2:40 AM Joel Borggrén-Franck
<joel.borggren.fra...@gmail.com> wrote:
>
> Hej Joe,
>
> New version looks good!
>
> Thanks for the explanations, makes sense to me.
>
> Cheers
> /Joel
>
> On Wed, 10 Oct 2018 at 08:27, joe darcy <joe.da...@oracle.com> wrote:
>>
>> Hi Joel,
>>
>> Thanks for the review; slightly revised version at
>>
>> http://cr.openjdk.java.net/~darcy/8058202.3/
>>
>> Comments below.
>>
>>
>> On 10/9/2018 11:00 AM, Joel Borggrén-Franck wrote:
>> > Hi Joe,
>> >
>> > Good to see this happening. In general looks good to me, a few nits below.
>> >
>> > AnnotatedTypeBaseImpl contains comments indicating from which
>> > superclass or interface the overridden methods comes. I'd either add
>> > // Object at line 207 or delete line 145 and 177 for consistency.
>>
>> Okay; comments added to follow that local convention.
>>
>> >
>> > Even though this isn't performance critical by far the allocation at
>> > line 215 still bugs me a bit given that the common case will most
>> > certainly be no annotations.
>>
>> Sure; refactored to avoid the allocation.
>>
>> >
>> > Why the inverted logic line 250-253, IIRC you should be able to test
>> > if it is an AnnotatedBaseTypeImpl, or?
>>
>> I was aiming to avoid testing for the impl class directly to not
>> directly tie the classes to this particular implementation of the
>> AnnotatedType hierarchy. However, inter-operability based on the specs
>> would need the equals and hashCode behavior to be defined.
>>
>> >
>> > For equalsTypeAndAnnotations and baseHashCode lines 232-244 equals
>> > test for owner type equality while hashcode doesn't include owner
>> > type. I must confess I no longer remember the equals-hashCode contract
>> > but I assume this is intentional.
>>
>> Good catch; equals and hashCode checks made consistent.
>>
>> Some refactoring and hardening of the test included too.
>>
>> Delta-diffs below.
>>
>> Thanks,
>>
>> -Joe
>>
>> diff
>> webrev.2/raw_files/new/src/java.base/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java
>> webrev/raw_files/new/src/java.base/share/classes/sun/reflect/annotation/AnnotatedTypeFactory.java
>>
>> 2c2
>> < * Copyright (c) 2013, 2015, Oracle and/or its affiliates. All rights
>> reserved.
>> ---
>> > * Copyright (c) 2013, 2018, Oracle and/or its affiliates. All rights
>> reserved.
>> 207c207
>> < @Override
>> ---
>> > @Override // java.lang.Object
>> 215d214
>> < StringJoiner sj = new StringJoiner(" ");
>> 216a216
>> > StringJoiner sj = new StringJoiner(" ");
>> 228,229c228,231
>> < }
>> < return sj.toString();
>> ---
>> > return sj.toString();
>> > } else {
>> > return "";
>> > }
>> 244c246,247
>> < Objects.hash((Object[])getAnnotations());
>> ---
>> > Objects.hash((Object[])getAnnotations()) ^
>> > Objects.hash(getAnnotatedOwnerType());
>>
>> diff
>> webrev.2/raw_files/new/test/jdk/java/lang/annotation/typeAnnotations/TestObjectMethods.java
>> webrev/raw_files/new/test/jdk/java/lang/annotation/typeAnnotations/TestObjectMethods.java
>>
>> 142d141
>> <
>> 157,168c156,158
>> < AnnotatedType annotType1 = m.getAnnotatedReturnType();
>> < AnnotatedType annotType2 = m.getAnnotatedReturnType();
>> <
>> < boolean valid = annotType1.equals(annotType2);
>> <
>> < if (!valid) {
>> < errors++;
>> < System.err.println(annotType1);
>> < System.err.println(" is not equal to ");
>> < System.err.println(annotType2);
>> < System.err.println();
>> < }
>> ---
>> > checkTypesForEquality(m.getAnnotatedReturnType(),
>> > m.getAnnotatedReturnType(),
>> > true);
>> 171a162,185
>> > private static void checkTypesForEquality(AnnotatedType annotType1,
>> > AnnotatedType annotType2,
>> > boolean expected) {
>> > boolean comparison = annotType1.equals(annotType2);
>> >
>> > if (comparison) {
>> > int hash1 = annotType1.hashCode();
>> > int hash2 = annotType1.hashCode();
>> > if (hash1 != hash2) {
>> > errors++;
>> > System.err.format("Equal AnnotatedTypes with unequal hash
>> codes: %n%s%n%s%n",
>> > annotType1.toString(), annotType2.toString());
>> > }
>> > }
>> >
>> > if (comparison != expected) {
>> > errors++;
>> > System.err.println(annotType1);
>> > System.err.println(expected ? " is not equal to " : " is
>> equal to ");
>> > System.err.println(annotType2);
>> > System.err.println();
>> > }
>> > }
>> >
>> 184,196c198,200
>> < AnnotatedType annotType1 =
>> methods[i].getAnnotatedReturnType();
>> < AnnotatedType annotType2 =
>> methods[j].getAnnotatedReturnType();
>> <
>> < boolean valid = !annotType1.equals(annotType2);
>> <
>> < if (!valid) {
>> < errors++;
>> < System.err.println(annotType1);
>> < System.err.println(" is equal to ");
>> < System.err.println(annotType2);
>> < System.err.println();
>> < }
>> <
>> ---
>> > checkTypesForEquality(methods[i].getAnnotatedReturnType(),
>> > methods[j].getAnnotatedReturnType(),
>> > false);
>> 209,210c213
>> < Method[] methods1 = clazz1.getDeclaredMethods();
>> < Method[] methods2 = clazz2.getDeclaredMethods();
>> ---
>> > System.err.println("Testing that presence/absence of
>> annotations matters for equals comparison.");
>> 212,226c215,230
>> < // Skip 0th element since void cannoted be annotated
>> < for (int i = 1; i < methods1.length; i++) {
>> < AnnotatedType annotType1 =
>> methods1[i].getAnnotatedReturnType();
>> < AnnotatedType annotType2 =
>> methods2[i].getAnnotatedReturnType();
>> <
>> < boolean valid = !annotType1.equals(annotType2);
>> <
>> < if (!valid) {
>> < errors++;
>> < System.err.println(annotType1);
>> < System.err.println(" is equal to ");
>> < System.err.println(annotType2);
>> < System.err.println();
>> < }
>> < }
>> ---
>> > String methodName = null;
>> > for (Method method : clazz1.getDeclaredMethods()) {
>> > if ("void".equals(method.getReturnType().toString())) {
>> > continue;
>> > }
>> >
>> > methodName = method.getName();
>> > try {
>> > checkTypesForEquality(method.getAnnotatedReturnType(),
>> > clazz2.getDeclaredMethod(methodName).getAnnotatedReturnType(),
>> > false);
>> > } catch (Exception e) {
>> > errors++;
>> > System.err.println("Method " + methodName + " not found.");
>> > }
>> > }
>> 230a235
>> > System.err.println("Testing wildcards");
>> 242,248c247
>> < if (awt1.equals(awt2)) {
>> < errors++;
>> < System.err.println(awt1);
>> < System.err.println(" is equal to ");
>> < System.err.println(awt2);
>> < System.err.println();
>> < }
>> ---
>> > checkTypesForEquality(awt1, awt2, false);
>> 254d252
>> <
--
http://www.google.com/profiles/wdietl
