RFR: 8300517: Refactor VisibleMemberTable (method members) [v2]

[Jonathan Gibbons]

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

>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 649:
>> 
>>> 647:             assert te.getKind().isClass();
>>> 648:             VisibleMemberTable vmt = configuration.getVisibleMemberTable(te);
>>> 649:             for (Element e : vmt.getMembers(VisibleMemberTable.Kind.METHODS)) {
>> 
>> In similar situations, I have found it useful to pass in a type token that determines the result type and which avoids casting the returned objects:
>> 
>> For example:
>> 
>>    <T> List<T extends Element> getMembers(VisibleMemberTable.Kind k, Class<T> clazz)
>
> Unfortunately, there's no such thing as generic enum, which would be just the ticket here.
> 
> Type token would be okay, but perhaps only marginally better than what we have now: neither option supports static checking. Anyhow, we might revisit it in the next round of refactoring, not now.

OK, with not now, but I do think the type token is marginally better than a cast of the result at the use site.

>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java line 1031:
>> 
>>> 1029:         // TODO: this computation should be eventually delegated to VisibleMemberTable
>>> 1030:         Set<TypeElement> alreadySeen = null;
>>> 1031:         assert (alreadySeen = new HashSet<>()) != null; // create set conditionally
>> 
>> I think this use of `assert` (here and lower down) is a step too far.  It's one thing to use asserts to verify invariants; it's too much to use them to conditionally compute state like this.
>
> That hash map is needed iff assertions are enabled. For an ordinary run, creating that hash map could be wasteful.
> 
> Perhaps I could create a hash map unconditionally using `HashMap.newHashMap(0)`; would that be to your liking?

The issue was more about the composite declaration and use of the asserts to build a collection solely for use in the assertions. This is a level of assert above simply checking invariants with no other side effects on the JVM.  The use is clever and not wrong, but it is uncommon.  Maybe a higher level comment might help, more than the low level "create set conditionally"

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

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