On Wed, 8 Mar 2023 14:42:46 GMT, Pavel Rappo <[email protected]> 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
