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
<
