On Wed, 1 Mar 2023 21:36:41 GMT, Paul Sandoz <psan...@openjdk.org> wrote:
>> Adam Sotona has updated the pull request incrementally with four additional
>> commits since the last revision:
>>
>> - renamed all remaining ConcreteXyzEntry to XyzEntryImpl
>> - abstract implementations of RefEntry, RefsEntry and NamedEntry renamed to
>> AbstractRefEntry, AbstractRefsEntry and AbstractNamedEntry
>> - renamed ConcreteBootstrapMethodEntry to BootstrapMethodEntryImpl
>> - ConcreteEntry renamed to AbstractPoolEntry
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
> line 48:
>
>> 46: default ConstantDesc constantValue() {
>> 47: return asSymbol();
>> 48: }
>
> It seems possible to use this pattern consistently for
> `ConstantDynamicEntry`, `MethodHandleEntry`, and `MethodTypeEntry`?
>
> At first i thought why not make `asSymbol` a covariant override of
> `constantValue`, but its not the same thing, since there are constant values
> (subtypes of `ConstantValueEntry`) that are not symbols.
Yes, I've moved the default `constantValue` delegation to `asSymbol` from
implementations up to the `ConstantDynamicEntry`, `MethodHandleEntry`, and
`MethodTypeEntry`.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ClassEntry.java
> line 71:
>
>> 69: * @return the combined {@link List}
>> 70: */
>> 71: static List<ClassEntry> adding(List<ClassEntry> base,
>> List<ClassEntry> additions) {
>
> This and all the other following static methods seem more like implementation
> details rather than part of the public API?
I've searched over all use cases we have and found no direct use of these API
methods, so I've removed them.
We may later re-open discussion about possible API to combine and deduplicate
lists of entries and symbols, however this isolated solution really does not
fit and does not serve any purpose.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPool.java
> line 49:
>
>> 47: import static jdk.internal.classfile.Classfile.TAG_STRING;
>> 48: import static jdk.internal.classfile.Classfile.TAG_UTF8;
>> 49:
>
> Unused imports, perhaps sweep through all classes (i think it can be done
> over multiple packages with IntelliJ).
This and hopefully all other unused imports have been removed.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
> line 222:
>
>> 220: * @param typeEntry the member field or method descriptor
>> 221: */
>> 222: NameAndTypeEntry natEntry(Utf8Entry nameEntry, Utf8Entry typeEntry);
>
> I would be inclined to rename more literally as `nameAndTypeEntry`, which is
> consistent with the naming convention in all other cases in this interface
> AFAICT (edit: almost i also see `bsm`) . (FWIW i keep reading nat as "not a
> type"!)
Good point, renamed.
Thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractPoolEntry.java
> line 376:
>
>> 374: } else if (o instanceof Utf8Entry u) {
>> 375: return equalsString(u.stringValue());
>> 376: }
>
> Dead branch? since there is only one concrete implementation of `Utf8Entry`.
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
> line 111:
>
>> 109: Set<ClassDesc> dependencies = cm.elementStream()
>> 110: .filter(ce -> ce instanceof
>> MethodModel)
>> 111: .flatMap(ce -> ((MethodModel)
>> ce).elementStream())
>
> You could potentially collapse this into a single `flatMap` and avoid the
> cast:
>
> .flatMap(ce -> ce instanceof MethodMethod mm ? mm.elementStream() :
> Stream.empty())
fixed, thanks.
-------------
PR: https://git.openjdk.org/jdk/pull/10982
