RFR: 8294982: Implementation of Classfile API [v31]

[Adam Sotona]

On Wed, 1 Mar 2023 21:36:41 GMT, Paul Sandoz <psandoz at 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