JDK 12 RFR of JDK-8058202 : AnnotatedType implementations don't override toString(), equals(), hashCode()
Joel Borggrén-Franck
joel.borggren.franck at gmail.com
Wed Oct 10 06:40:04 UTC 2018
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.darcy at 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
> <
>
More information about the core-libs-dev
mailing list