Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 23:07:38 GMT, Paul Sandoz 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/ClassReader.java line 144:
>
>> 142: * constant pool size, or zero
>> 143: * @throws ClassCastException if the index does not correspond to
>> 144: * a module entry
>
> Throwing `ClassCastException` is certainly convenient from an implementation
> perspective, but I think it is better to throw `IllegalArgumentException`.
Fixed in all `ClassReader::readXyzEntry` methods, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 90:
>
>> 88: private BootstrapMethodsAttribute bootstrapMethodsAttribute;
>> 89:
>> 90: @SuppressWarnings("unchecked")
>
> Is this needed?
not needed, removed, thanks
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 115:
>
>> 113:
>> 114: // 4
>> 115: case TAG_CONSTANTDYNAMIC, TAG_FIELDREF, TAG_FLOAT,
>> TAG_INTEGER, TAG_INTERFACEMETHODREF, TAG_INVOKEDYNAMIC, TAG_METHODREF,
>> TAG_NAMEANDTYPE -> p += 4;
>
> Break the line
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
> line 132:
>
>> 130: this.cp = new PoolEntry[constantPoolCount];
>> 131:
>> 132: p = metadataStart;
>
> Redundant assignment (see line 127).
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 23:43:55 GMT, Paul Sandoz 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/attribute/CodeAttribute.java
> line 56:
>
>> 54: * @param label a marker for a position within this {@code
>> CodeAttribute}
>> 55: * @return position of the {@code Label} in the {@code codeArray}
>> 56: */
>
> Suggestion:
>
> /**
> * {@return the position of the {@code Label} in the {@code codeArray}}
> * @param label a marker for a position within this {@code CodeAttribute}
> */
>
> Throws IAE if the label is not positioned in the code array?
All the dependent code expects -1 when the Label is not positioned in the code
array.
Throwing IAE would require significant refactoring and may have performance
effects.
I'll add a javadoc comment meanwhile.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 22:38:32 GMT, Paul Sandoz 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/impl/SplitConstantPool.java
> line 167:
>
>> 165: buf.patchInt(pos + 2, 4, attrLen - 6);
>> 166: buf.patchInt(pos + 6, 2, bsmSize);
>> 167: return true;
>
> The if and else branch return true, factor out at the end of the method?
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 339:
>
>> 337: }
>> 338:
>> 339: private AbstractPoolEntry.Utf8EntryImpl tryFindUtf8(int hash,
>> String target) {
>
> Unused type variable `T`
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
> line 488:
>
>> 486: return methodHandleEntry(refKind, reference);
>> 487: }
>> 488: return internalAdd(new
>> AbstractPoolEntry.MethodHandleEntryImpl(this, size, hash, refKind,
>> (AbstractPoolEntry.AbstractMemberRefEntry) reference), hash);
>
> Break the long line (same for two following methods).
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 21:36:41 GMT, Paul Sandoz 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 adding(List base,
>> List 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 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
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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/ClassModel.java line 86:
> 84: * {@snippet lang=java :
> 85: * Classfile.build(thisClass(), ConstantPoolBuilder.of(this),
> 86: * b -> b.transform(this, transform);
Suggestion:
* Classfile.build(thisClass(), ConstantPoolBuilder.of(this),
* b -> b.transform(this, transform));
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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/attribute/CodeAttribute.java
line 56:
> 54: * @param label a marker for a position within this {@code
> CodeAttribute}
> 55: * @return position of the {@code Label} in the {@code codeArray}
> 56: */
Suggestion:
/**
* {@return the position of the {@code Label} in the {@code codeArray}}
* @param label a marker for a position within this {@code CodeAttribute}
*/
Throws IAE if the label is not positioned in the code array?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > 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/ClassReader.java line 144: > 142: * constant pool size, or zero > 143: * @throws ClassCastException if the index does not correspond to > 144: * a module entry Throwing `ClassCastException` is certainly convenient from an implementation perspective, but I think it is better to throw `IllegalArgumentException`. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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/ClassReader.java line 270:
> 268: int classReaderOffset,
> 269: int length);
> 270: }
Suggestion:
}
src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
line 90:
> 88: private BootstrapMethodsAttribute bootstrapMethodsAttribute;
> 89:
> 90: @SuppressWarnings("unchecked")
Is this needed?
src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
line 115:
> 113:
> 114: // 4
> 115: case TAG_CONSTANTDYNAMIC, TAG_FIELDREF, TAG_FLOAT,
> TAG_INTEGER, TAG_INTERFACEMETHODREF, TAG_INVOKEDYNAMIC, TAG_METHODREF,
> TAG_NAMEANDTYPE -> p += 4;
Break the line
src/java.base/share/classes/jdk/internal/classfile/impl/ClassReaderImpl.java
line 132:
> 130: this.cp = new PoolEntry[constantPoolCount];
> 131:
> 132: p = metadataStart;
Redundant assignment (see line 127).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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/impl/SplitConstantPool.java
line 167:
> 165: buf.patchInt(pos + 2, 4, attrLen - 6);
> 166: buf.patchInt(pos + 6, 2, bsmSize);
> 167: return true;
The if and else branch return true, factor out at the end of the method?
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 339:
> 337: }
> 338:
> 339: private AbstractPoolEntry.Utf8EntryImpl tryFindUtf8(int hash,
> String target) {
Unused type variable `T`
src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java
line 488:
> 486: return methodHandleEntry(refKind, reference);
> 487: }
> 488: return internalAdd(new
> AbstractPoolEntry.MethodHandleEntryImpl(this, size, hash, refKind,
> (AbstractPoolEntry.AbstractMemberRefEntry) reference), hash);
Break the long line (same for two following methods).
src/java.base/share/classes/jdk/internal/classfile/impl/TemporaryConstantPool.java
line 56:
> 54: public final class TemporaryConstantPool implements ConstantPoolBuilder {
> 55:
> 56: private TemporaryConstantPool() {};
Suggestion:
private TemporaryConstantPool() {}
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > 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/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"!) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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 71:
> 69: * @return the combined {@link List}
> 70: */
> 71: static List adding(List base,
> List additions) {
This and all the other following static methods seem more like implementation
details rather than part of the public API?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > 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/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). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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/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`.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote:
>> This is root pull request with Classfile API implementation, tests and
>> benchmarks initial drop into JDK.
>>
>> Following pull requests consolidating JDK class files parsing, generating,
>> and transforming
>> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to
>> this one.
>>
>> Classfile API development is tracked at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch
>>
>> Development branch of consolidated JDK class files parsing, generating, and
>> transforming is at:
>> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch
>>
>> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online
>> API
>> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html)
>> is also available.
>>
>> Please take you time to review this non-trivial JDK addition.
>>
>> Thank you,
>> Adam
>
> 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.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
On Wed, 1 Mar 2023 15:00:41 GMT, Adam Sotona wrote: >> This is root pull request with Classfile API implementation, tests and >> benchmarks initial drop into JDK. >> >> Following pull requests consolidating JDK class files parsing, generating, >> and transforming >> ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) will chain to >> this one. >> >> Classfile API development is tracked at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch >> >> Development branch of consolidated JDK class files parsing, generating, and >> transforming is at: >> https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch >> >> Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online >> API >> documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) >> is also available. >> >> Please take you time to review this non-trivial JDK addition. >> >> Thank you, >> Adam > > 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/snippet-files/PackageSnippets.java line 111: > 109: Set 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()) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v31]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam 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 - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/212bb04e..1e95e508 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=30 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=29-30 Stats: 2758 lines in 32 files changed: 1283 ins; 1285 del; 190 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
