[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-compiler-dev
Subject:    Re: RFR: 8284220: TypeMirror#toString omits enclosing class names after JDK-8281238 [v4]
From:       Liam Miller-Cushon <cushon () openjdk ! java ! net>
Date:       2022-04-26 17:11:46
Message-ID: QNIEeGSA75JKnPZE95_fzLYjioFddxggq1xfqEqM9yw=.9bf05ffe-3c05-454b-b2e3-ac90053756ef () github ! com
[Download RAW message or body]

On Tue, 26 Apr 2022 04:35:49 GMT, Joe Darcy <darcy@openjdk.org> wrote:

> > Liam Miller-Cushon has updated the pull request with a new target base due to a \
> > merge or a rebase. The incremental webrev excludes the unrelated changes brought \
> > in by the merge/rebase. The pull request contains three additional commits since \
> > the last revision: 
> > - Merge branch 'openjdk:master' into JDK-8284220
> > - Refactor test based on review feedback
> > - 8284220: TypeMirror#toString omits enclosing class names after JDK-8281238
> 
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Type.java line 1046:
> 
> > 1044:                         buf.append(".");
> > 1045:                     }
> > 1046:                     ListBuffer<Name> names = new ListBuffer<>();
> 
> There may be a more idiomataic way to javac to construct this string, but offhand I \
> don't know what it is.

I made this as idiomatic as I could, I think part of the issue here is that that the \
pretty-printing logic is operating on names at a slightly lower level than we \
normally do, so the existing ways of getting simple or qualified names aren't \
sufficient. Suggestions are welcome if you think of a way to clean this up at all.

> test/langtools/tools/javac/processing/model/type/AnnotatedTypeToString/AnnotatedTypeToString.java \
> line 1: 
> > 1: /*
> 
> From personal experience with similar tests, they can look obscure when revisting \
> them months in the future. I suggest putting a sentence or two description of what \
> the test is trying to do. Also, if all the types can fit in one file, that can be \
> easier to understand.

Thanks, I added some more context to the test `@summary` and also added javadoc on \
the processor, suggestions for additional documentation improvements are welcome.

> if all the types can fit in one file, that can be easier to understand.

I considered that, but `JavacTestingAbstractProcessor` is in the default packages, \
and I want the test to exercise printing types that are not in the default package, \
so I don't see a good way to put everything in the same file.

-------------

PR: https://git.openjdk.java.net/jdk/pull/8084


[prev in list] [next in list] [prev in thread] [next in thread]