On Thu, 12 Jan 2023 10:25:27 GMT, Maurizio Cimadamore <mcimadam...@openjdk.org>
wrote:
>> Archie L. Cobbs has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Use the more appropriate Type comparison method Types.isSameType().
>> - Add some more comments to clarify how the analysis works.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 218:
>
>> 216: new TreeScanner() {
>> 217:
>> 218: private Lint lint = ThisEscapeAnalyzer.this.lint;
>
> On a first look I'm not sure about the granularity of suppression here. I
> believe that suppressing at the class level, or at the constructor level is
> enough. Allowing to further annotate var declarations and non-constructor
> methods, while doable, might actually be counter productive - in the sense
> that the annotation does not occur in the constructor (where you'd want to
> see it) but in some other method. I think the fact that a constructor is
> escaping (willingly) should be a visible thing. Something to consider.
Two things...
We need to support annotations on field declarations because their initializers
are effectively mini-constructors. But we don't need it on local variables,
parameters, etc. - too confusing.
For annotations on methods, yes it's debatable. It can be handy if you have
e.g. an `init()` method that all of your constructors invoke. However, I agree
it's also confusing, so I will remove.
But we should keep the notion that if a constructor invokes `this()`, and the
invoked constructor has annotations suppressed, then we skip over the
constructor invocation.
I will make these updates.
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/ThisEscapeAnalyzer.java
> line 304:
>
>> 302:
>> 303: // We are looking for analyzable constructors only
>> 304: final Symbol sym = entry.getKey();
>
> This seems unused. And, if so, perhaps we only need a `Set<MethodInfo>`, not
> a map.
Thanks, you're right, the map keys are not used here. I'll clean up the loop.
But we still need a map, not a set, because the map is used elsewhere.
-------------
PR: https://git.openjdk.org/jdk/pull/11874
