On Thu, 4 Jun 2026 03:39:24 GMT, Chen Liang <[email protected]> wrote:
>> Vicente Romero has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review comments
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/LocalProxyVarsGen.java
> line 131:
>
>> 129: earlyReads.addAll(fieldsReadInPrologue.get(tree));
>> 130: }
>> 131: fieldsReadInPrologue.remove(tree);
>
> Suggestion:
>
> Set<Symbol> constructorEarlyReads =
> fieldsReadInPrologue.remove(tree);
> if (earlyReads == null) {
> earlyReads = constructorEarlyReads;
> } else {
> earlyReads.addAll(constructorEarlyReads);
> }
I agree this can be consolidated a bit -- using `remove` to avoid `get` +
`remove` is nice. My general feeling is that the null logic is a bit confusing
as well. E.g. maybe starting with an empty set and calling addAll a couple of
times is clearer -- too bad that we have to deal with nulls from remove.
This might be nicer:
Stream.of(classDecl, tree)
.flatMap(this::earlyReadsForTree)
.collect(Collectors.toSet());
With a helper method:
Set<Symbol> earlyReadsForTree(JCTree tree) {
Set<Symbol> earlyReads = fieldsReadInPrologue.remove(tree);
return earlyReads == null ? earlyReads : Set.of();
}
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2507#discussion_r3354747574
