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 > < >
