RFR: 8303755: Clean up around JavacElements.getAllMembers [v3]

[Jan Lahoda]

On Wed, 8 Mar 2023 10:14:21 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/util/Iterators.java line 68:
>> 
>>> 66:         @Override
>>> 67:         public O next() {
>>> 68:             if (!hasNext()) {
>> 
>> I was looking into the history, and the reason for the current code is JDK-8167070 (performance). So, I'd suggest to try how this new code performs. The pre-JDK-8167070 did two calls to nested Iterator's hasNext for each call to hasNext (so for deeper hierarchies, the number of invocations grew very fast), while the code here only does one, so it quite probably may be OK.
>> 
>> I wonder if it is time to write a (unit) test for this class?
>
> When I saw a _custom_ empty iterator, my first reaction was to ask myself why it was needed when Collections.emptyIterator had been long available. So yes, I also saw JDK-8167070.
> 
> I think, I was careful enough to preserve the good behavior and remove the bad behavior as well as unneeded code. When developing this PR, I used throwaway unit tests to satisfy myself that the iterator behaves as expected. I'm not sure that we need to integrate those tests. But if we do, then please suggest how it should look like, infrastructure-wise that is.

A test could be something like this:

/*
 * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
 * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 *
 * This code is free software; you can redistribute it and/or modify it
 * under the terms of the GNU General Public License version 2 only, as
 * published by the Free Software Foundation.
 *
 * This code is distributed in the hope that it will be useful, but WITHOUT
 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 * version 2 for more details (a copy is included in the LICENSE file that
 * accompanied this code).
 *
 * You should have received a copy of the GNU General Public License version
 * 2 along with this work; if not, write to the Free Software Foundation,
 * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
 *
 * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
 * or visit www.oracle.com if you need additional information or have any
 * questions.
 */

/**
 * @test
 * @bug 8303755
 * @summary Verify that Iterators method work as expected
 * @modules jdk.compiler/com.sun.tools.javac.util
 * @run junit IteratorsTest
 */

import com.sun.tools.javac.util.Iterators;
import java.util.Iterator;
import java.util.List;
import java.util.function.Function;
import org.junit.jupiter.api.*;

public class IteratorsTest {

    @Test
    public void compoundIterator() {
        TestConverter<TestIterator<String>, String> c = new TestConverter<>(it -> it);
        TestIterator<String> test1 = new TestIterator<>(List.of("1").iterator());
        TestIterator<String> test2 = new TestIterator<>(List.of("2").iterator());
        Iterator<String> compound = Iterators.createCompoundIterator(List.of(test1, test2), c);

        //nothing should be called before the hasNext or next is called:
        assertMaxCalls(c, 0);
        assertMaxCalls(test1, 0, 0);
        assertMaxCalls(test2, 0, 0);

        //when hasNext is called, should invoke the hasNext delegate once:
        Assertions.assertTrue(compound.hasNext());

        assertMaxCalls(c, 1);
        assertMaxCalls(test1, 1, 0);
        assertMaxCalls(test2, 0, 0);

        Assertions.assertTrue(compound.hasNext());

        assertMaxCalls(c, 0);
        assertMaxCalls(test1, 1, 0);
        assertMaxCalls(test2, 0, 0);

        //next may invoke hasNext once:
        Assertions.assertEquals("1", compound.next());

        assertMaxCalls(c, 0);
        assertMaxCalls(test1, 1, 1);
        assertMaxCalls(test2, 0, 0);
    }

    private void assertMaxCalls(TestIterator<?> test, int maxExpectedHasNextCalls, int maxExpectedNextCalls) {
        if (test.hasNextCalls > maxExpectedHasNextCalls) {
            Assertions.fail("too many hasNext invocations: " + test.hasNextCalls +
                            ", expected: " + maxExpectedHasNextCalls);
        }
        test.hasNextCalls = 0;
        if (test.nextCalls > maxExpectedNextCalls) {
            Assertions.fail("too many next invocations: " + test.nextCalls +
                            ", expected: " + maxExpectedNextCalls);
        }
        test.nextCalls = 0;
    }

    private void assertMaxCalls(TestConverter<?, ?> test, int maxExpectedApplyCalls) {
        if (test.applyCalls > maxExpectedApplyCalls) {
            Assertions.fail("too many apply invocations: " + test.applyCalls +
                            ", expected: " + maxExpectedApplyCalls);
        }
        test.applyCalls = 0;
    }

    class TestIterator<T> implements Iterator<T> {
        int hasNextCalls;
        int nextCalls;
        final Iterator<T> delegate;

        public TestIterator(Iterator<T> delegate) {
            this.delegate = delegate;
        }

        @Override
        public boolean hasNext() {
            hasNextCalls++;
            return delegate.hasNext();
        }

        @Override
        public T next() {
            nextCalls++;
            return delegate.next();
        }
    }
    class TestConverter<I, O>  implements Function<I, Iterator<O>> {
        int applyCalls;
        final Function<I, Iterator<O>> delegate;

        public TestConverter(Function<I, Iterator<O>> delegate) {
            this.delegate = delegate;
        }

        @Override
        public Iterator<O> apply(I t) {
            applyCalls++;
            return delegate.apply(t);
        }
    }
}


I might be able to improve it later, if needed.

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

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