Re: RFR: 8294982: Implementation of Classfile API [v36]
On Tue, 7 Mar 2023 08:35:34 GMT, Adam Sotona wrote: >> I am unsure how you might use `BlockCodeBuilder`. If the current signature >> is not changed then `transformFromHandler` seems reasonable (since its the >> handler that pushes elements into its given builder). >> >> The other `transform` is implicitly "transform model". > > I'm sorry my answer was inaccurate, what I'm proposing is > `transformBlock(Consumer, CodeTransform)` or > `transformedBlock(Consumer, CodeTransform)`. > `BlockCodeBuilder` is just an extension of `CodeBuilder` with ability to > break block execution. > Block is a sub-unit of code, so I expect it would be a logical extension of > the actual `block(Consumer`. > I'll move this discussion to the mailing list to make common decision about > API change. > Thanks. This topic is now tracked as RFE - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 15:51:25 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/NewObjectInstruction.java
>> line 38:
>>
>>> 36: * of a {@link CodeModel}.
>>> 37: */
>>> 38: public sealed interface NewObjectInstruction extends Instruction
>>
>> Should we add a helper method on `CodeBuilder` that does the new + dup +
>> invoke special dance?
>
> That is great RFE for `CodeBuilder`, thanks.
collected and tracked in the mailing list
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 22:05:24 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
> line 156:
>
>> 154: @Override
>> 155: public String toString() {
>> 156: return String.format("Store[OP=%s, slot=%d]",
>> this.opcode(), slot());
>
> A suggestion. I believe the `toString` output for bound and unbound
> instructions should be identical and it can composed solely from methods on
> the public instruction interface. There are some differences e.g. `Increment`
> and `Inc` for the unbound and bound increment instruction respectively.
>
> If those assumptions are correct i recommend placing a static `toString`
> method on all unbound instructions that also have bound instructions,
> accepting the public instruction interface as an argument. Then the
> overridden `Object::toString` method defers to those. Thereby there is no
> duplication.
> (Alas we cannot override `toString` on the instruction interface itself).
resolved using common static format Strings
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 23:49:24 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/CodeTransform.java line 91:
>
>> 89: @Override
>> 90: default ResolvedTransform resolve(CodeBuilder builder) {
>> 91: return new TransformImpl.CodeTransformImpl(e -> accept(builder,
>> e),
>
> Make `CodeTransformImpl` generic in the class file element, rename to say
> `ResolvedTransformImpl` and reuse for the other `XxxTransform`?
Good idea, I'll fix it, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 16:33:49 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java
>> line 63:
>>
>>> 61: private static final Runnable NOTHING = () -> { };
>>> 62:
>>> 63: interface FakeClassTransform extends ClassTransform {
>>
>> Rename to `UnresolvedXxxTransform`? I think that is a better name, since it
>> could appear in stack traces. Like with `XxxTransformImpl` it may be
>> possible to share across all implementations by mixing in?
>
> Renamed to `UnresolvedXyzTransform`, thanks.
> I'll consider conversion to generic form in a next step.
Unfortunately application of common generic `UnresolvedTransform` does
not work here.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Mon, 6 Mar 2023 17:45:27 GMT, Paul Sandoz wrote: >> I see what you mean. `transforming` was selected to represent the >> continuation of the code building process, similar to `trying` and >> `catching`. While `transform` may imply that the actual code builder gets >> transformed into something else. For example `MethodModel::transformCode` >> takes `CodeModel`, applies `CodeTransform` and after that the code of the >> actual method is finished. >> What about `transformBlock(BlockCodeBuilder, CodeTransform)`? > > I am unsure how you might use `BlockCodeBuilder`. If the current signature is > not changed then `transformFromHandler` seems reasonable (since its the > handler that pushes elements into its given builder). > > The other `transform` is implicitly "transform model". I'm sorry my answer was inaccurate, what I'm proposing is `transformBlock(Consumer, CodeTransform)` or `transformedBlock(Consumer, CodeTransform)`. `BlockCodeBuilder` is just an extension of `CodeBuilder` with ability to break block execution. Block is a sub-unit of code, so I expect it would be a logical extension of the actual `block(Consumer`. I'll move this discussion to the mailing list to make common decision about API change. Thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Mon, 6 Mar 2023 17:34:35 GMT, Adam Sotona wrote: >> The use-case seems fine to me (and that it only makes sense for building >> code). I still think it's a "transform", but with a different source. Subtly >> changing the name makes it seem different and fundamentally it is not >> AFAICT. If there is a separate name I think it should reflect the difference >> in source input to the transformation, rather than differentiate via the >> present participle. > > I see what you mean. `transforming` was selected to represent the > continuation of the code building process, similar to `trying` and > `catching`. While `transform` may imply that the actual code builder gets > transformed into something else. For example `MethodModel::transformCode` > takes `CodeModel`, applies `CodeTransform` and after that the code of the > actual method is finished. > What about `transformBlock(BlockCodeBuilder, CodeTransform)`? I am unsure how you might use `BlockCodeBuilder`. If the current signature is not changed then `transformFromHandler` seems reasonable (since its the handler that pushes elements into its given builder). The other `transform` is implicitly "transform model". - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
The message from this sender included one or more files which could not be scanned for virus detection; do not open these files unless you are certain of the sender's intent. -- On Mon, 6 Mar 2023 17:15:25 GMT, Paul Sandoz wrote: >> The `CodeBuilder::transforming` solves a bit different use cases than all >> the other transform. >> It is designed to be able to use code transformations on a code building >> handler within a single pass. >> Main reason is support of features like `StackTracker` in a form of code >> transformation. `StackTracker` (or any other similar tool requiring to >> monitor or affect code building) is passed as a transformation of a code >> fragment, while it can immediately serve as a source of information >> necessary to generate follow-up bytecode of the same method (in the same >> pass). >> Example of such use case is here: >> https://github.com/openjdk/jdk/blob/0e43af667ba6c6bda61461c260688bc46d3f3474/src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java#L49 >> >> These code generation/transformation cases must be handled in a single pass >> and `CodeBuilder::transforming` method has no similar peer in any other >> (method, field or class) builder, because it is not necessary. > > The use-case seems fine to me (and that it only makes sense for building > code). I still think it's a "transform", but with a different source. Subtly > changing the name makes it seem different and fundamentally it is not AFAICT. > If there is a separate name I think it should reflect the difference in > source input to the transformation, rather than differentiate via the present > participle. I see what you mean. `transforming` was selected to represent the continuation of the code building process, similar to `trying` and `catching`. While `transform` may imply that the actual code builder gets transformed into something else. For example `MethodModel::transformCode` takes `CodeModel`, applies `CodeTransform` and after that the code of the actual method is finished. What about `transformBlock(BlockCodeBuilder, CodeTransform)`? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 19:56:08 GMT, Paul Sandoz wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> StackMapFrameInfo extracted to top level from StackMapTableAttribute > > src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java > line 149: > >> 147: var instrumentorCodeMap = instrumentor.methods().stream() >> 148: >> .filter(instrumentedMethodsFilter) >> 149: >> .collect(Collectors.toMap(mm -> mm.methodName().stringValue() + >> mm.methodType().stringValue(), mm -> mm.code().orElse(null))); > > Should we be filtering out abstract methods before the `collect` and/or > replace with: > > mm -> mm.code().orElseThrow() > > ? Abstract methods should not be selected for instrumentation, so `orElseThrow()` is a good idea. Fixed, thanks. > src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java > line 171: > >> 169: >> 170: //store stacked method >> parameters into locals >> 171: var storeStack = new >> LinkedList(); > > FWIW you can use an ArrayDeque + push + forEach, in the spirit of > highlighting Deque over LinkedList for stack-based usage (since this is an > example with visibility). fixed, thanks. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Mon, 6 Mar 2023 17:02:46 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 165:
>>
>>> 163: * @return this builder
>>> 164: */
>>> 165: default CodeBuilder transforming(CodeTransform transform,
>>> Consumer handler) {
>>
>> The functionality of this method, `transforming`, and
>> `ClassfileBuilder::transform`, are in effect equivalent in their
>> transforming: adding the results of transformed code to the builder. They
>> differ in the source of code elements.
>>
>> The latter's behaviour can be implemented using the former, with a consumer
>> that passes all elements of a code model to the builder e.g. `builder ->
>> model.forEach(builder::with)`.
>>
>> The difference in naming initially confused me. To me this suggests the
>> method names should be the same? (perhaps with the transformer being
>> consistently the last argument?).
>
> The `CodeBuilder::transforming` solves a bit different use cases than all the
> other transform.
> It is designed to be able to use code transformations on a code building
> handler within a single pass.
> Main reason is support of features like `StackTracker` in a form of code
> transformation. `StackTracker` (or any other similar tool requiring to
> monitor or affect code building) is passed as a transformation of a code
> fragment, while it can immediately serve as a source of information necessary
> to generate follow-up bytecode of the same method (in the same pass).
> Example of such use case is here:
> https://github.com/openjdk/jdk/blob/0e43af667ba6c6bda61461c260688bc46d3f3474/src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java#L49
>
> These code generation/transformation cases must be handled in a single pass
> and `CodeBuilder::transforming` method has no similar peer in any other
> (method, field or class) builder, because it is not necessary.
The use-case seems fine to me (and that it only makes sense for building code).
I still think it's a "transform", but with a different source. Subtly changing
the name makes it seem different and fundamentally it is not AFAICT. If there
is a separate name I think it should reflect the difference in source input to
the transformation, rather than differentiate via the present participle.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 23:28:23 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 165:
>
>> 163: * @return this builder
>> 164: */
>> 165: default CodeBuilder transforming(CodeTransform transform,
>> Consumer handler) {
>
> The functionality of this method, `transforming`, and
> `ClassfileBuilder::transform`, are in effect equivalent in their
> transforming: adding the results of transformed code to the builder. They
> differ in the source of code elements.
>
> The latter's behaviour can be implemented using the former, with a consumer
> that passes all elements of a code model to the builder e.g. `builder ->
> model.forEach(builder::with)`.
>
> The difference in naming initially confused me. To me this suggests the
> method names should be the same? (perhaps with the transformer being
> consistently the last argument?).
The `CodeBuilder::transforming` solves a bit different use cases than all the
other transform.
It is designed to be able to use code transformations on a code building
handler within a single pass.
Main reason is support of features like `StackTracker` in a form of code
transformation. `StackTracker` (or any other similar tool requiring to monitor
or affect code building) is passed as a transformation of a code fragment,
while it can immediately serve as a source of information necessary to generate
follow-up bytecode of the same method (in the same pass).
Example of such use case is here:
https://github.com/openjdk/jdk/blob/0e43af667ba6c6bda61461c260688bc46d3f3474/src/java.base/share/classes/jdk/internal/classfile/components/CodeStackTracker.java#L49
These code generation/transformation cases must be handled in a single pass and
`CodeBuilder::transforming` method has no similar peer in any other (method,
field or class) builder, because it is not necessary.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 00:57:35 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AbstractDirectBuilder.java
> line 34:
>
>> 32: * AbstractDirectBuilder
>> 33: */
>> 34: public class AbstractDirectBuilder {
>
> Type variable `B` is unused.
fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java
> line 63:
>
>> 61: private static final Runnable NOTHING = () -> { };
>> 62:
>> 63: interface FakeClassTransform extends ClassTransform {
>
> Rename to `UnresolvedXxxTransform`? I think that is a better name, since it
> could appear in stack traces. Like with `XxxTransformImpl` it may be possible
> to share across all implementations by mixing in?
Renamed to `UnresolvedXyzTransform`, thanks.
I'll consider conversion to generic form in a next step.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 15:50:33 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/NewMultiArrayInstruction.java
>> line 60:
>>
>>> 58: static NewMultiArrayInstruction of(ClassEntry arrayTypeEntry,
>>> 59:int dimensions) {
>>> 60: return new
>>> AbstractInstruction.UnboundNewMultidimensionalArrayInstruction(arrayTypeEntry,
>>> dimensions);
>>
>> Should we validate that the dimensionality of `arrayType` is greater than or
>> equal to `dimensions`?
>
> Architectural decision is to do not provide much of validation in favour of
> performance, however it might be re-visited in cases like this. Please raise
> the validation topic at classfile-api-dev at openjdk.org, thanks.
Ok.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 20:54:39 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/LoadInstruction.java
> line 66:
>
>> 64: * @param slot the local varaible slot to load from
>> 65: */
>> 66: static LoadInstruction of(Opcode op, int slot) {
>
> Should you validate that `slot` is compatible with `op`? (same for the
> StoreInstruction?)
There are three aspects of this kind of validation:
1. similar to `arrayType` there was an architectural decision to less validate
so to perform faster
2. static instruction factory does not have the information necessary to
validate local variable slot, it could theoretically be validated in the
`CodeBuilder`
3. incompatible slot would most probably prevent stack map generation and such
code will fail in the later phase, or at least when verification is explicitly
called on the generated code
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 20:37:37 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/NewMultiArrayInstruction.java
> line 60:
>
>> 58: static NewMultiArrayInstruction of(ClassEntry arrayTypeEntry,
>> 59:int dimensions) {
>> 60: return new
>> AbstractInstruction.UnboundNewMultidimensionalArrayInstruction(arrayTypeEntry,
>> dimensions);
>
> Should we validate that the dimensionality of `arrayType` is greater than or
> equal to `dimensions`?
Architectural decision is to do not provide much of validation in favour of
performance, however it might be re-visited in cases like this. Please raise
the validation topic at classfile-api-dev at openjdk.org, thanks.
> src/java.base/share/classes/jdk/internal/classfile/instruction/NewObjectInstruction.java
> line 38:
>
>> 36: * of a {@link CodeModel}.
>> 37: */
>> 38: public sealed interface NewObjectInstruction extends Instruction
>
> Should we add a helper method on `CodeBuilder` that does the new + dup +
> invoke special dance?
That is great RFE for `CodeBuilder`, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Fri, 3 Mar 2023 14:53:19 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
>> line 42:
>>
>>> 40: * the setting of the {@link Classfile.Option#processDebug(boolean)}
>>> option.
>>> 41: */
>>> 42: public sealed interface CharacterRange extends PseudoInstruction
>>
>> This and some other instructions with unbounded implementations do not have
>> static `of` factory methods. Is that intended?
>
> It seems to be incomplete. I'll add factory methods to `CharacterRange`,
> `LineNumber`, `LocalVariable` and `LocalVariableType`.
> Thanks for pointing it out.
Fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 19:31:50 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
> line 98:
>
>> 96: @Override
>> 97: public void accept(CodeBuilder cob, CodeElement coe) {
>> 98: switch (coe) {
>
> Missing case for`CharacterRange` instruction? I am not sure it makes any
> sense, if so perhaps add a comment as to why.
Fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 19:27:58 GMT, Paul Sandoz wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> StackMapFrameInfo extracted to top level from StackMapTableAttribute
>
> src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
> line 42:
>
>> 40: * the setting of the {@link Classfile.Option#processDebug(boolean)}
>> option.
>> 41: */
>> 42: public sealed interface CharacterRange extends PseudoInstruction
>
> This and some other instructions with unbounded implementations do not have
> static `of` factory methods. Is that intended?
It seems to be incomplete. I'll add factory methods to `CharacterRange`,
`LineNumber`, `LocalVariable` and `LocalVariableType`.
Thanks for pointing it out.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/TypeKind.java line 45:
> 43: DoubleType("double", "D", 7),
> 44: /** a reference type */
> 45: ReferenceType("reference type", "A", -1),
Suggestion:
ReferenceType("reference type", "L", -1),
?
Otherwise `TypeKind.fromDescriptor(TypeKind.ReferenceType.descriptor())` will
fail.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/NonterminalCodeBuilder.java
line 46:
> 44: this.terminal = switch (parent) {
> 45: case ChainedCodeBuilder cb -> cb.terminal;
> 46: case BlockCodeBuilderImpl cb -> cb.terminal;
Suggestion:
case NonterminalCodeBuilder cb -> cb.terminal;
?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractDirectBuilder.java
line 34:
> 32: * AbstractDirectBuilder
> 33: */
> 34: public class AbstractDirectBuilder {
Type variable `B` is unused.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/TransformImpl.java line
63:
> 61: private static final Runnable NOTHING = () -> { };
> 62:
> 63: interface FakeClassTransform extends ClassTransform {
Rename to `UnresolvedXxxTransform`? I think that is a better name, since it
could appear in stack traces.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeTransform.java line 91:
> 89: @Override
> 90: default ResolvedTransform resolve(CodeBuilder builder) {
> 91: return new TransformImpl.CodeTransformImpl(e -> accept(builder,
> e),
Make `CodeTransformImpl` generic in the class file element, rename to say
`ResolvedTransformImpl` and reuse for the other `XxxTransform`?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 165:
> 163: * @return this builder
> 164: */
> 165: default CodeBuilder transforming(CodeTransform transform,
> Consumer handler) {
The functionality of this method, `transforming`, and
`ClassfileBuilder::transform`, are in effect equivalent in their transforming:
adding the results of transformed code to the builder. They differ in the
source of code elements.
The latter's behaviour can be implemented using the former, with a consumer
that passes all elements of a code model to the builder e.g. `builder ->
model.forEach(builder::with)`.
The difference in naming initially confused me. To me this suggests the method
names should be the same? (perhaps with the transformer being consistently the
last argument?).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 164:
> 162: * generate the code.
> 163: * @return this builder
> 164: */
Suggestion:
/**
* Apply a transform to the code built by a handler, directing results to
this builder.
*
* @param transform the transform to apply to the code built by the handler
* @param handler the handler that receives a {@linkplain CodeBuilder} to
* build the code.
* @return this builder
*/
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java line 93:
> 91: * directly; they are passed to handlers by methods such as {@link
> 92: * MethodBuilder#withCode(Consumer)} or to code transforms. The elements
> of a
> 93: * code can be specified abstractly (by passing a {@link CodeElement} to
> {@link
Suggestion:
* code can be specified abstractly, by passing a {@link CodeElement} to {@link
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
line 156:
> 154: @Override
> 155: public String toString() {
> 156: return String.format("Store[OP=%s, slot=%d]", this.opcode(),
> slot());
A suggestion. I believe the `toString` output for bound and unbound
instructions should be identical and it can composed solely from methods on the
public instruction interface. There are some differences e.g. `Increment` and
`Inc` for the unbound and bound increment instruction respectively.
If those assumptions are correct i recommend placing a static `toString` method
on all unbound instructions that also have bound instructions, accepting the
public instruction interface as an argument. Then the overridden
`Object::toString` method defers to those. Thereby there is no duplication.
(Alas we cannot override `toString` on the instruction interface itself).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/LoadInstruction.java
line 66:
> 64: * @param slot the local varaible slot to load from
> 65: */
> 66: static LoadInstruction of(Opcode op, int slot) {
Should you validate that `slot` is compatible with `op`? (same for the
StoreInstruction?)
src/java.base/share/classes/jdk/internal/classfile/instruction/NewObjectInstruction.java
line 38:
> 36: * of a {@link CodeModel}.
> 37: */
> 38: public sealed interface NewObjectInstruction extends Instruction
Should we add a helper method on `CodeBuilder` that does the new + dup + invoke
special dance?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/NewMultiArrayInstruction.java
line 60:
> 58: static NewMultiArrayInstruction of(ClassEntry arrayTypeEntry,
> 59:int dimensions) {
> 60: return new
> AbstractInstruction.UnboundNewMultidimensionalArrayInstruction(arrayTypeEntry,
> dimensions);
Should we validate that the dimensionality of `arrayType` is greater than or
equal to `dimensions`?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/ExceptionCatch.java
line 77:
> 75: static ExceptionCatch of(Label handler, Label tryStart, Label tryEnd,
> 76: Optional catchTypeEntry) {
> 77: return new AbstractPseudoInstruction.ExceptionCatchImpl(handler,
> tryStart, tryEnd, catchTypeEntry);
Suggestion:
return new AbstractPseudoInstruction.ExceptionCatchImpl(handler,
tryStart, tryEnd, catchTypeEntry.orElse(null));
Then you only need one constructor for `ExceptionCatchImpl` and you don't need
to disambiguate a call using a cast for a final argument that is `null` .
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 41:
> 39: * Models a constant-load instruction in the {@code code} array of a
> {@code
> 40: * Code} attribute, including "intrinsic constant" instructions (e.g.,
> {@code
> 41: * aload_0}), "argument constant" instructions (e.g., {@code bipush}),
> and "load
Suggestion:
* iconst_0}), "argument constant" instructions (e.g., {@code bipush}), and
"load
Otherwise we can refer to `aconst_null`
src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java
line 60:
> 58: /**
> 59: * Models an "intrinsic constant" instruction (e.g., {@code
> 60: * aload_0}).
Suggestion:
* iconst_0}).
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional > commit since the last revision: > > StackMapFrameInfo extracted to top level from StackMapTableAttribute src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java line 149: > 147: var instrumentorCodeMap = instrumentor.methods().stream() > 148: > .filter(instrumentedMethodsFilter) > 149: > .collect(Collectors.toMap(mm -> mm.methodName().stringValue() + > mm.methodType().stringValue(), mm -> mm.code().orElse(null))); Should we be filtering out abstract methods before the `collect` and/or replace with: mm -> mm.code().orElseThrow() ? - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional > commit since the last revision: > > StackMapFrameInfo extracted to top level from StackMapTableAttribute src/java.base/share/classes/jdk/internal/classfile/components/snippet-files/PackageSnippets.java line 171: > 169: > 170: //store stacked method > parameters into locals > 171: var storeStack = new > LinkedList(); FWIW you can use an ArrayDeque + push + forEach, in the spirit of highlighting Deque over LinkedList for stack-based usage (since this is an example with visibility). - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/instruction/CharacterRange.java
line 42:
> 40: * the setting of the {@link Classfile.Option#processDebug(boolean)}
> option.
> 41: */
> 42: public sealed interface CharacterRange extends PseudoInstruction
This and some other instructions with unbounded implementations do not have
static `of` factory methods. Is that intended?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/components/CodeRelabeler.java
line 98:
> 96: @Override
> 97: public void accept(CodeBuilder cob, CodeElement coe) {
> 98: switch (coe) {
Missing case for`CharacterRange` instruction? I am not sure it makes any sense,
if so perhaps add a comment as to why.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
On Thu, 2 Mar 2023 14:31:06 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 one additional
> commit since the last revision:
>
> StackMapFrameInfo extracted to top level from StackMapTableAttribute
src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java
line 157:
> 155: if (!(ce instanceof
> MethodModel mm
> 156: &&
> mm.methodName().stringValue().startsWith("debug")))
> 157: classBuilder.with(ce);
Indentation is confusing (even though its less concise its probably better to
use braces for all the examples):
Suggestion:
classBuilder.with(ce);
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v36]
> 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 one additional commit since the last revision: StackMapFrameInfo extracted to top level from StackMapTableAttribute - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/13d78c96..074dd304 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=35 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=34-35 Stats: 225 lines in 6 files changed: 123 ins; 93 del; 9 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
