RFR: 8303882: Refactor some iterators in jdk.compiler [v3]

Maurizio Cimadamore mcimadamore at openjdk.org
Mon Mar 13 10:52:34 UTC 2023


On Mon, 13 Mar 2023 09:55:02 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> Please review this refactoring to iterators in jdk.compiler. The refactoring delegates more to collections framework, while retaining the performance characteristics of bespoke iterators. The refactoring also adds inline comments, `@Override` annotations, and fixes some trivial bugs (as can be seen in the added test, which is slightly augmented from that suggested by Jan Lahoda here https://github.com/openjdk/jdk/pull/12904#discussion_r1129777660).
>> 
>> I'll add a comment here once I have benchmarked the change formally.
>
> Pavel Rappo has updated the pull request incrementally with four additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'myjdk/8303882' into 8303882
>  - Squeeze a few extra drops of perf
>  - Add a recursive test
>  - Try squeeze more performance

Looks good - some minor (optional) comments on the test

test/langtools/tools/javac/util/IteratorsTest.java line 112:

> 110:         assertAndResetMaxCalls(c, 0);
> 111:         assertAndResetMaxCalls(test1, 1, 1);
> 112:         assertAndResetMaxCalls(test2, 0, 0);

Should the test continue, and check that we end up with calls to the second iterator?

test/langtools/tools/javac/util/IteratorsTest.java line 163:

> 161: 
> 162:         public TestConverter(Function<I, Iterator<O>> delegate) {
> 163:             this.delegate = delegate;

This parameter doesn't really seem to be used in the test?

test/langtools/tools/javac/util/IteratorsTest.java line 172:

> 170:         }
> 171:     }
> 172: }

Missing newline here

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

Marked as reviewed by mcimadamore (Reviewer).

PR: https://git.openjdk.org/jdk/pull/12949


More information about the compiler-dev mailing list