Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional > commits since the last revision: > > - fixed AccessFlags javadoc > - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform > - removed obsolete generic parameter from AbstractDirectBuilder This is a significant amount of work over a long period of time. Well done to all involved. Initially it's easy to get overwhelmed with the quantity of new classes. There are lots of "things" in a classfile and to properly model a classfile there will naturally be lots of classes for those "things". It's all very logically structured and very disciplined. For me the most powerful aspect of the design is that of the builder, which can be combined with a flatmap transformation and pattern matching. A clean, functional, and immutable design. When transforming a classfile the developer has a choice to apply composed transformations to a stream of elements or obtain a list of all elements and apply more globally (perhaps building up a transformation plan that is then applied to the stream), depending on their needs. It's slightly awkward that composed transformers (resulting from `andThen` and `ofStateful`) cannot be operated on and the resolved artifact is exposed, but i don't think anyone will likely notice given the usage. (I cannot off the top of my head suggest anything better. Maybe as this soaks internally new ideas emerge.) - Marked as reviewed by psandoz (Reviewer). PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 161:
> 159: var desc = cd.descriptorString();
> 160: return switch (desc.charAt(0)) {
> 161: case '[' -> desc;
Is this correct? Arrays don't have an internal name.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/CodeImpl.java line 52:
> 50: static final Instruction[] SINGLETON_INSTRUCTIONS = new
> Instruction[256];
> 51:
> 52: static {
Can we loop through all `Opcode` values filter for `sizeIfFixed == 1` and
switch on the kind? If so that would avoid the need for explicit lists and
simplify the code.
src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line 176:
> 174: }
> 175:
> 176: public static long nextPowerOfTwo( long x ) {
If you like you can change the implementation to be:
x = -1 >>> Long.numberOfLeadingZeros(x - 1);
return x + 1;
See `ConcurrentHashMap`.
src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 84:
> 82: }
> 83: }
> 84: }
Suggestion:
loop: for (int i = 1; i < type.length(); ++i) {
switch (type.charAt(i)) {
case '[':
bs.set(i);
while (type.charAt(++i) == '[')
;
if (type.charAt(i) == 'L') {
while (type.charAt(++i) != ';')
;
}
break;
case ')':
break loop;
default:
bs.set(i);
if (type.charAt(i) == 'L') {
while (type.charAt(++i) != ';')
;
}
}
}
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional > commits since the last revision: > > - fixed AccessFlags javadoc > - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform > - removed obsolete generic parameter from AbstractDirectBuilder src/java.base/share/classes/jdk/internal/classfile/impl/ClassPrinterImpl.java line 570: > 568: fieldToTree(f, verbosity > 569: .with(new ListNodeImpl(BLOCK, "methods", > clm.methods().stream().map(mm -> > 570: (Node)methodToTree(mm, verbosity; Suggestion: methodToTree(mm, verbosity; (Note as i go through the code i am looking at the IDEs report of problems and making note as comments. Likely be more productive for you to do that directly.) - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional > commits since the last revision: > > - fixed AccessFlags javadoc > - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform > - removed obsolete generic parameter from AbstractDirectBuilder src/java.base/share/classes/jdk/internal/classfile/impl/BufferedMethodBuilder.java line 54: > 52: private final List elements = new ArrayList<>(); > 53: private final SplitConstantPool constantPool; > 54: private final ClassEntry thisClass; Unused. Can be removed as can the associated constructor parameter. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/BootstrapMethodEntryImpl.java
line 83:
> 81: }
> 82: else
> 83: return false;
Suggestion:
return obj instanceof BootstrapMethodEntry e
&& e.bootstrapMethod().equals(handle)
&& e.arguments().equals(arguments);
? I am uncertain about the use of reference equality, since it indicates an
instance is only equal to itself?
src/java.base/share/classes/jdk/internal/classfile/impl/BootstrapMethodEntryImpl.java
line 91:
> 89: for (LoadableConstantEntry a : arguments) {
> 90: hash = 31 * hash + a.hashCode();
> 91: }
Suggestion:
hash = 31 * hash + arguments.hashCode();
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
line 73:
> 71: if (a.attributeMapper() == am)
> 72: iterator.remove();
> 73: }
Suggestion:
attributes.removeIf(a -> a.attributeMappter() == am);
But presumably use an inner class instead. I can understand because of that if
you want to keep the existing code instead.
src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
line 76:
> 74: }
> 75:
> 76: List> attributes() {
Unused
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/AnnotationReader.java
line 99:
> 97: }
> 98:
> 99: public static List>
> readParameterAnnotations(ClassReader classReader, int p, boolean isVisible) {
Parameter `isVisible` is unused, but method is called with true/false values.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v43]
On Fri, 3 Mar 2023 16:39: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 three additional
> commits since the last revision:
>
> - fixed AccessFlags javadoc
> - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform
> - removed obsolete generic parameter from AbstractDirectBuilder
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractInstruction.java
line 683:
> 681: public UnboundInstruction(Opcode op, int size) {
> 682: super(op, size);
> 683: }
Unused?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls [v2]
On Fri, 3 Mar 2023 07:30:39 GMT, Kim Barrett wrote: >> David Holmes has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Restrict getaddrinfo to IPv4 only as per the rest of the code > > Looks good. Thanks for the reviews @kimbarrett and @djelinski ! I'll leave this till early next week to integrate. - PR: https://git.openjdk.org/jdk/pull/12842
Re: RFR: JDK-8301998: Update HarfBuzz to 7.0.1 [v2]
On Fri, 3 Mar 2023 20:30:23 GMT, Phil Race wrote: >> Harshitha Onkar has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Updating.txt changes > > src/java.desktop/share/native/libharfbuzz/hb-icu.h line 1: > >> 1: /* > > We don't need/use hb-icu.h and hb-icu.cc, so why are they here ? > They were there in harfbuzz source for a long time and we've never needed > them before and > given what their purpose is, I don't see why we need them now. > Are you sure you followed the process of adding in ONLY new files which were > needed so that we successfully > compiler ? > Please double check ALL new files you added are really needed. @prrace hb-icu.h was referenced in hb-unicode.cc (existing file), but I missed to see that it is included only when certain flags are set. Thank you for catching it. I will double check all the newly added files again. - PR: https://git.openjdk.org/jdk/pull/12864
Re: RFR: JDK-8301998: Update HarfBuzz to 7.0.1 [v2]
On Fri, 3 Mar 2023 20:25:09 GMT, Harshitha Onkar wrote: >> HarfBuzz library updated from v4.4.1 to v7.0.1 >> >> - harfbuzz.md file updated >> - Added -DHB_NO_VISIBILITY to HARFBUZZ_CFLAGS in Awt2DLibraries.gmk to avoid >> build issues on linux and macos targets. >> - GPOS.hh has been moved from Layout to Layout/GPOS to match upstream >> changes. >> - Total of 270 files changed out of which 46 are newly added. > > Harshitha Onkar has updated the pull request incrementally with one > additional commit since the last revision: > > Updating.txt changes src/java.desktop/share/native/libharfbuzz/hb-icu.h line 1: > 1: /* We don't need/use hb-icu.h and hb-icu.cc, so why are they here ? They were there in harfbuzz source for a long time and we've never needed them before and given what their purpose is, I don't see why we need them now. Are you sure you followed the process of adding in ONLY new files which were needed so that we successfully compiler ? Please double check ALL new files you added are really needed. - PR: https://git.openjdk.org/jdk/pull/12864
Re: RFR: JDK-8301998: Update HarfBuzz to 7.0.1 [v2]
> HarfBuzz library updated from v4.4.1 to v7.0.1 > > - harfbuzz.md file updated > - Added -DHB_NO_VISIBILITY to HARFBUZZ_CFLAGS in Awt2DLibraries.gmk to avoid > build issues on linux and macos targets. > - GPOS.hh has been moved from Layout to Layout/GPOS to match upstream changes. > - Total of 270 files changed out of which 46 are newly added. Harshitha Onkar has updated the pull request incrementally with one additional commit since the last revision: Updating.txt changes - Changes: - all: https://git.openjdk.org/jdk/pull/12864/files - new: https://git.openjdk.org/jdk/pull/12864/files/e6f0cbc5..6db2d186 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12864=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12864=00-01 Stats: 21 lines in 1 file changed: 21 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12864.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12864/head:pull/12864 PR: https://git.openjdk.org/jdk/pull/12864
RFR: JDK-8301998: Update HarfBuzz to 7.0.1
HarfBuzz library updated from v4.4.1 to v7.0.1 - harfbuzz.md file updated - Added -DHB_NO_VISIBILITY to HARFBUZZ_CFLAGS in Awt2DLibraries.gmk to avoid build issues on linux and macos targets. - GPOS.hh has been moved from Layout to Layout/GPOS to match upstream changes. - Total of 270 files changed out of which 46 are newly added. - Commit messages: - harfbuzz.md updated - newly added files & folders - harfbuzz updated files Changes: https://git.openjdk.org/jdk/pull/12864/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12864=00 Issue: https://bugs.openjdk.org/browse/JDK-8301998 Stats: 40669 lines in 271 files changed: 26593 ins; 8274 del; 5802 mod Patch: https://git.openjdk.org/jdk/pull/12864.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12864/head:pull/12864 PR: https://git.openjdk.org/jdk/pull/12864
Re: RFR: 8303588: [JVMCI] make JVMCI source directories conform with standard layout
On Fri, 3 Mar 2023 17:47:20 GMT, Doug Simon wrote: > The layout of the sources in `jdk.internal.vm.ci` stems from their initial > development outside the JDK where they adopted a layout influenced by Eclipse. > > There's no good reason for maintaining this layout any more. Moving to a > standard layout also means IDEs will be able to make sense of the JVMCI > sources in `src.zip`. Good. Please, test it in mach5. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.org/jdk/pull/12860
RFR: 8303588: [JVMCI] make JVMCI source directories conform with standard layout
The layout of the sources in `jdk.internal.vm.ci` stems from their initial development outside the JDK where they adopted a layout influenced by Eclipse. There's no good reason for maintaining this layout any more. Moving to a standard layout also means IDEs will be able to make sense of the JVMCI sources in `src.zip`. - Commit messages: - made JVMCI source directories conform with standard layout Changes: https://git.openjdk.org/jdk/pull/12860/files Webrev: https://webrevs.openjdk.org/?repo=jdk=12860=00 Issue: https://bugs.openjdk.org/browse/JDK-8303588 Stats: 20 lines in 209 files changed: 0 ins; 20 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12860.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12860/head:pull/12860 PR: https://git.openjdk.org/jdk/pull/12860
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 [v43]
> 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 three additional commits since the last revision: - fixed AccessFlags javadoc - TransformImpl.FakeXyzTransform renamed to UnresolvedXyzTransform - removed obsolete generic parameter from AbstractDirectBuilder - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/8561d134..173dc1e7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=42 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=41-42 Stats: 24 lines in 7 files changed: 0 ins; 0 del; 24 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
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 [v41]
On Fri, 3 Mar 2023 14:14:55 GMT, Jaikiran Pai wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Signature.TypeArg does not inherit from Signature
>
> src/java.base/share/classes/jdk/internal/classfile/AccessFlags.java line 54:
>
>> 52: * {@return whether the specified flag is present} The specified
>> flag
>> 53: * should be a valid flag for the classfile location associated with
>> this
>> 54: * element.
>
> Hello Adam, the way this is worded, it feels like if the flag isn't valid
> then this method would raise an exception. Looking at the implementation,
> that doesn't look like the case. Should it be reworded to say it returns
> false in such cases?
>
> On a related note, since this JEP is for introducing this API for internal
> use only, would you prefer if the javadoc text isn't reviewed to this level
> of detail?
I'll fix it, thanks for the review.
Manageable amount of javadoc comments is OK, preferably with directly proposed
patches :)
-
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 [v42]
> 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 two additional commits since the last revision: - factory methods to CharacterRange, LineNumber, LocalVariable and LocalVariableType - CodeRelabeler fix - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/b03f15c1..8561d134 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=41 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=40-41 Stats: 107 lines in 6 files changed: 100 ins; 3 del; 4 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
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 [v41]
On Fri, 3 Mar 2023 13:50:15 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:
>
> Signature.TypeArg does not inherit from Signature
src/java.base/share/classes/jdk/internal/classfile/AccessFlags.java line 54:
> 52: * {@return whether the specified flag is present} The specified flag
> 53: * should be a valid flag for the classfile location associated with
> this
> 54: * element.
Hello Adam, the way this is worded, it feels like if the flag isn't valid then
this method would raise an exception. Looking at the implementation, that
doesn't look like the case. Should it be reworded to say it returns false in
such cases?
On a related note, since this JEP is for introducing this API for internal use
only, would you prefer if the javadoc text isn't reviewed to this level of
detail?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v41]
> 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: Signature.TypeArg does not inherit from Signature - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/79bfd4c5..b03f15c1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=40 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=39-40 Stats: 78 lines in 5 files changed: 37 ins; 8 del; 33 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
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Fri, 3 Mar 2023 10:09:27 GMT, Claes Redestad wrote: > Yes, iff means if-and-only-if and is used for extra precision in formal > logic, mathematics. I've never come across it before. With your explanations, it makes perfect sense. - PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls [v2]
On Fri, 3 Mar 2023 09:56:35 GMT, David Holmes wrote: >> We can replace `gethostbyname`, which is deprecated on Windows and Linux, >> with `getaddrinfo`. This API is available on all supported platforms and so >> can be placed in shared code. @djelinski pointed out that `getaddrinfo` can >> resolve both IP addresses and host names so the two step approach used in >> `networkStream::connect` is not necessary and we can do away with >> `os::get_host_by_name()` completely. >> >> The build is updated to enable winsock deprecation warnings, and now we need >> to use `ws2_32.lib` we can drop `wsock32.lib` (as it is basically a subset - >> again thanks @djelinski ). >> >> Testing >> - all Oracle builds in tiers 1-5 >> - All GHA builds >> >> The actual code change has to be manually tested because the code is only >> used by Ideal Graph Printing to connect to the Ideal Graph Visualizer. I've >> manually tested on Windows and Linux and @tobiasholenstein tested macOS. >> >> Thanks. > > David Holmes has updated the pull request incrementally with one additional > commit since the last revision: > > Restrict getaddrinfo to IPv4 only as per the rest of the code Marked as reviewed by djelinski (Committer). - PR: https://git.openjdk.org/jdk/pull/12842
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Fri, 3 Mar 2023 09:38:13 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java
>> line 257:
>>
>>> 255:
>>> 256: /**
>>> 257: * @return true iff the BSM method type exactly matches
>>
>> I assume “iff” should “if”?
>
> Here and elsewhere in this file "iff" might mean [if and only
> if](https://en.wikipedia.org/wiki/If_and_only_if), which would make sense.
> (FWIW, there are a few hundred occurrences of the word "iff" in src.)
>
> @cl4es (Claes Redestad), as the author of those lines would you like to chime
> in?
>
> Since Claes might read this, I note that when I changed unsupported `{@see}`
> to `{@link}` thoughtout this file, my IDE could not resolve one of the links:
> `java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,Class,MethodType,MethodHandle,MethodType)`
>
> While there's a similarly-name method with slightly different parameters, I
> refrained from using it:
> `java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,MethodType,MethodType,MethodHandle,MethodType)`.
Yes, iff means if-and-only-if and is used for extra precision in formal logic,
mathematics. As @pavelrappo points out it's a relatively common occurrence in
the OpenJDK sources, though perhaps not in the public javadocs. Perhaps a bit
pretentious, but mostly a terse way to say "return true if the BSM method type
exactly matches X, otherwise false".
The broken link stems from the fact that the method I was targeting (a way to
use condy for lambda proxy singletons rather than a `MethodHandle.constant`)
was never integrated. We'll look at either getting that done (@briangoetz
suggested the time might be ready for it) or remove this currently pointless
static bootstrap specialization test.
-
PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8286781: Replace the deprecated/obsolete gethostbyname and inet_addr calls [v2]
> We can replace `gethostbyname`, which is deprecated on Windows and Linux, > with `getaddrinfo`. This API is available on all supported platforms and so > can be placed in shared code. @djelinski pointed out that `getaddrinfo` can > resolve both IP addresses and host names so the two step approach used in > `networkStream::connect` is not necessary and we can do away with > `os::get_host_by_name()` completely. > > The build is updated to enable winsock deprecation warnings, and now we need > to use `ws2_32.lib` we can drop `wsock32.lib` (as it is basically a subset - > again thanks @djelinski ). > > Testing > - all Oracle builds in tiers 1-5 > - All GHA builds > > The actual code change has to be manually tested because the code is only > used by Ideal Graph Printing to connect to the Ideal Graph Visualizer. I've > manually tested on Windows and Linux and @tobiasholenstein tested macOS. > > Thanks. David Holmes has updated the pull request incrementally with one additional commit since the last revision: Restrict getaddrinfo to IPv4 only as per the rest of the code - Changes: - all: https://git.openjdk.org/jdk/pull/12842/files - new: https://git.openjdk.org/jdk/pull/12842/files/81f15e05..8b6f6317 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=12842=01 - incr: https://webrevs.openjdk.org/?repo=jdk=12842=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12842.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12842/head:pull/12842 PR: https://git.openjdk.org/jdk/pull/12842
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Fri, 3 Mar 2023 08:15:49 GMT, Alexey Ivanov wrote:
>> Please review this superficial documentation cleanup that was triggered by
>> unrelated analysis of doc comments in JDK API.
>>
>> The only effect that this multi-area PR has on the JDK API Documentation
>> (i.e. the observable effect on the generated HTML pages) can be summarized
>> as follows:
>>
>>
>> diff -ur
>> build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> 2023-03-02 11:47:44
>> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> 2023-03-02 11:48:45
>> @@ -17084,7 +17084,7 @@
>> throws > href="java.base/java/io/IOException.html" title="class in
>> java.io">IOException,
>> ClassNotFoundException
>> readObject is called to restore the
>> state of the
>> - (@code BasicPermission} from a stream.
>> + BasicPermission from a stream.
>>
>> Parameters:
>> s - the ObjectInputStream from which data
>> is read
>>
>> Notes
>> -
>>
>> * I'm not an expert in any of the affected areas, except for jdk.javadoc,
>> and I was merely after misused tags. Because of that, I would appreciate
>> reviews from experts in other areas.
>> * I discovered many more issues than I included in this PR. The excluded
>> issues seem to occur in infrequently updated third-party code (e.g.
>> javax.xml), which I assume we shouldn't touch unless necessary.
>> * I will update copyright years after (and if) the fix had been approved, as
>> required.
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2866:
>
>> 2864: * Merge multiple abstract methods. The preferred method is a
>> method that is a subsignature
>> 2865: * of all the other signatures and whose return type is more
>> specific {@link MostSpecificReturnCheck}.
>> 2866: * The resulting preferred method has a thrown clause that is the
>> intersection of the merged
>
> Is it “…has a {@code throws} clause…”?
Thanks! I'll add this to a separate PR.
-
PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 16:23:17 GMT, Alexey Ivanov wrote:
>> Please review this superficial documentation cleanup that was triggered by
>> unrelated analysis of doc comments in JDK API.
>>
>> The only effect that this multi-area PR has on the JDK API Documentation
>> (i.e. the observable effect on the generated HTML pages) can be summarized
>> as follows:
>>
>>
>> diff -ur
>> build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
>> 2023-03-02 11:47:44
>> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
>> 2023-03-02 11:48:45
>> @@ -17084,7 +17084,7 @@
>> throws > href="java.base/java/io/IOException.html" title="class in
>> java.io">IOException,
>> ClassNotFoundException
>> readObject is called to restore the
>> state of the
>> - (@code BasicPermission} from a stream.
>> + BasicPermission from a stream.
>>
>> Parameters:
>> s - the ObjectInputStream from which data
>> is read
>>
>> Notes
>> -
>>
>> * I'm not an expert in any of the affected areas, except for jdk.javadoc,
>> and I was merely after misused tags. Because of that, I would appreciate
>> reviews from experts in other areas.
>> * I discovered many more issues than I included in this PR. The excluded
>> issues seem to occur in infrequently updated third-party code (e.g.
>> javax.xml), which I assume we shouldn't touch unless necessary.
>> * I will update copyright years after (and if) the fix had been approved, as
>> required.
>
> src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
> 257:
>
>> 255:
>> 256: /**
>> 257: * @return true iff the BSM method type exactly matches
>
> I assume “iff” should “if”?
Here and elsewhere in this file "iff" might mean [if and only
if](https://en.wikipedia.org/wiki/If_and_only_if), which would make sense.
(FWIW, there are a few hundred occurrences of the word "iff" in src.)
@cl4es (Claes Redestad), as the author of those lines would you like to chime
in?
Since Claes might read this, I note that when I changed unsupported `{@see}` to
`{@link}` thoughtout this file, my IDE could not resolve one of the links:
`java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,Class,MethodType,MethodHandle,MethodType)`
While there's a similarly-name method with slightly different parameters, I
refrained from using it:
`java.lang.invoke.LambdaMetafactory#metafactory(MethodHandles.Lookup,String,MethodType,MethodType,MethodHandle,MethodType)`.
-
PR: https://git.openjdk.org/jdk/pull/12826
Re: RFR: 8294982: Implementation of Classfile API [v40]
> 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: Update src/java.base/share/classes/jdk/internal/classfile/TypeKind.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/e5fd5764..79bfd4c5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=39 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=38-39 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 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
Re: RFR: 8294982: Implementation of Classfile API [v39]
> 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: Update src/java.base/share/classes/jdk/internal/classfile/impl/NonterminalCodeBuilder.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/0bd5281f..e5fd5764 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=38 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=37-38 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 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
Re: RFR: 8294982: Implementation of Classfile API [v38]
> 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 three additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/CodeBuilder.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/instruction/ExceptionCatch.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/02bd6dcb..0bd5281f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=37 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=36-37 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 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
Re: RFR: 8294982: Implementation of Classfile API [v37]
> 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 three additional commits since the last revision: - Update src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/instruction/ConstantInstruction.java Co-authored-by: Paul Sandoz - Update src/java.base/share/classes/jdk/internal/classfile/snippet-files/PackageSnippets.java Co-authored-by: Paul Sandoz - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/074dd304..02bd6dcb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=10982=36 - incr: https://webrevs.openjdk.org/?repo=jdk=10982=35-36 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 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
Re: RFR: 8303480: Miscellaneous fixes to mostly invisible doc comments
On Thu, 2 Mar 2023 12:03:44 GMT, Pavel Rappo wrote:
> Please review this superficial documentation cleanup that was triggered by
> unrelated analysis of doc comments in JDK API.
>
> The only effect that this multi-area PR has on the JDK API Documentation
> (i.e. the observable effect on the generated HTML pages) can be summarized as
> follows:
>
>
> diff -ur build/macosx-aarch64/images/docs-before/api/serialized-form.html
> build/macosx-aarch64/images/docs-after/api/serialized-form.html
> --- build/macosx-aarch64/images/docs-before/api/serialized-form.html
> 2023-03-02 11:47:44
> +++ build/macosx-aarch64/images/docs-after/api/serialized-form.html
> 2023-03-02 11:48:45
> @@ -17084,7 +17084,7 @@
> throws href="java.base/java/io/IOException.html" title="class in
> java.io">IOException,
> ClassNotFoundException
> readObject is called to restore the
> state of the
> - (@code BasicPermission} from a stream.
> + BasicPermission from a stream.
>
> Parameters:
> s - the ObjectInputStream from which data
> is read
>
> Notes
> -
>
> * I'm not an expert in any of the affected areas, except for jdk.javadoc, and
> I was merely after misused tags. Because of that, I would appreciate reviews
> from experts in other areas.
> * I discovered many more issues than I included in this PR. The excluded
> issues seem to occur in infrequently updated third-party code (e.g.
> javax.xml), which I assume we shouldn't touch unless necessary.
> * I will update copyright years after (and if) the fix had been approved, as
> required.
Looks good to me.
I looked through all the changes, paying more attention to the client area.
src/java.base/share/classes/java/lang/invoke/BootstrapMethodInvoker.java line
257:
> 255:
> 256: /**
> 257: * @return true iff the BSM method type exactly matches
I assume “iff” should “if”?
src/jdk.compiler/share/classes/com/sun/tools/javac/code/Types.java line 2866:
> 2864: * Merge multiple abstract methods. The preferred method is a
> method that is a subsignature
> 2865: * of all the other signatures and whose return type is more
> specific {@link MostSpecificReturnCheck}.
> 2866: * The resulting preferred method has a thrown clause that is the
> intersection of the merged
Is it “…has a {@code throws} clause…”?
-
Marked as reviewed by aivanov (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12826
