Re: RFR: 8341064: Define anchor point and index term for "wrapper classes" [v3]
On Sat, 28 Sep 2024 21:45:09 GMT, Chen Liang wrote:
>> src/java.base/share/classes/java/lang/package-info.java line 34:
>>
>>> 32: * Frequently it is necessary to represent a value of primitive
>>> 33: * type as if it were an object. The
>>> 34: * {@index "wrapper classes"} {@link Boolean}, {@link
>>
>> I can see that JDK uses the `` pattern in many locations.
>> That is, an empty `a` tag which has the `name` attribute. You use the `id`
>> attribute. JDK uses such pattern too, but only in a few locations.
>>
>> Regardless of the details, I was wondering if instead of adding a new tag
>> with an attribute, we should just add an attribute to an existing tag.
>>
>> `dfn` seems to be designed exactly for that [^1][^2]. Additionally, keeping
>> tags to a minimum might not worsen readability as much:
>>
>> {@index "wrapper classes"}
>>
>> @jonathan-gibbons, @hns, thoughts?
>>
>> [^1]:
>> https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-dfn-element
>> [^2]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dfn
>
> Agree; I have recommedned consolidating id property into h2 without an extra
> a tag.
`` is a semantic tag to indicate the defining instance of a term. It may
be used by search engines, to improve their results. When `` is used as
intended, it may be reasonable and convenient to put an `id` on the tag, to
provide a link target for elsewhere in the documentation. It may also be
convenient to add `id` to other locations, especially headers, but note that
`javadoc` now does that automatically.
The usage of `` is a legacy usage from HTML 4, before the
improved rules for `id` in HTML 5. It would be a reasonable cleanup to move
away from such tags, putting an equivalent `id` on either a replacement tag
(such as ``) or on an appropriate nearby tag.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/21215#discussion_r1780100498
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v15]
On Fri, 20 Sep 2024 14:02:14 GMT, Nizar Benalla wrote: >> This checker checks the values of the `@since` tag found in the >> documentation comment for an element against the release in which the >> element first appeared. >> >> Real since value of an API element is computed as the oldest release in >> which the given API element was introduced. That is: >> - for modules, classes and interfaces, the release in which the element with >> the given qualified name was introduced >> - for constructors, the release in which the constructor with the given VM >> descriptor was introduced >> - for methods and fields, the release in which the given method or field >> with the given VM descriptor became a member of its enclosing class or >> interface, whether direct or inherited >> >> Effective since value of an API element is computed as follows: >> - if the given element has a `@since` tag in its javadoc, it is used >> - in all other cases, return the effective since value of the enclosing >> element >> >> The since checker verifies that for every API element, the real since value >> and the effective since value are the same, and reports an error if they are >> not. >> >> Preview method are handled as per JEP 12, if `@PreviewFeature` is used >> consistently going forward then the checker doesn't need to be updated with >> every release. The checker has explicit knowledge of preview elements that >> came before `JDK 14` because they weren't marked in a machine understandable >> way and preview elements that came before `JDK 17` that didn't have >> `@PreviewFeature`. >> >> Important note : We only check code written since `JDK 9` as the releases >> used to determine the expected value of `@since` tags are taken from the >> historical data built into `javac` which only goes back that far >> >> The intial comment at the beginning of `SinceChecker.java` holds more >> information into the program. >> >> I already have filed issues and fixed some wrong tags like in #18640, >> #18032, #18030, #18055, #18373, #18954, #18972. > > Nizar Benalla has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 31 commits: > > - exclude list is now module specific > - remove empty if statement, debugging showed the condition is never true > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Change "found" to "should be" in the error messages > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - avoid using `continue`, fix mistake in last commit > - extend SinceChecker to now skip some packages > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - ... and 21 more: https://git.openjdk.org/jdk/compare/3c22d83c...62a34064 Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2318971590
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v14]
On Thu, 19 Sep 2024 16:38:54 GMT, Nizar Benalla wrote: >> This checker checks the values of the `@since` tag found in the >> documentation comment for an element against the release in which the >> element first appeared. >> >> Real since value of an API element is computed as the oldest release in >> which the given API element was introduced. That is: >> - for modules, classes and interfaces, the release in which the element with >> the given qualified name was introduced >> - for constructors, the release in which the constructor with the given VM >> descriptor was introduced >> - for methods and fields, the release in which the given method or field >> with the given VM descriptor became a member of its enclosing class or >> interface, whether direct or inherited >> >> Effective since value of an API element is computed as follows: >> - if the given element has a `@since` tag in its javadoc, it is used >> - in all other cases, return the effective since value of the enclosing >> element >> >> The since checker verifies that for every API element, the real since value >> and the effective since value are the same, and reports an error if they are >> not. >> >> Preview method are handled as per JEP 12, if `@PreviewFeature` is used >> consistently going forward then the checker doesn't need to be updated with >> every release. The checker has explicit knowledge of preview elements that >> came before `JDK 14` because they weren't marked in a machine understandable >> way and preview elements that came before `JDK 17` that didn't have >> `@PreviewFeature`. >> >> Important note : We only check code written since `JDK 9` as the releases >> used to determine the expected value of `@since` tags are taken from the >> historical data built into `javac` which only goes back that far >> >> The intial comment at the beginning of `SinceChecker.java` holds more >> information into the program. >> >> I already have filed issues and fixed some wrong tags like in #18640, >> #18032, #18030, #18055, #18373, #18954, #18972. > > Nizar Benalla has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 28 commits: > > - Change "found" to "should be" in the error messages > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - avoid using `continue`, fix mistake in last commit > - extend SinceChecker to now skip some packages > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Skip over files and packages that aren't found > - Move the tests to module/module_name directory for clearer naming > - (C) > - ... and 18 more: https://git.openjdk.org/jdk/compare/bc36ace7...d355222e Generally very good. It's getting to be a really nice piece of functionality. I added one suggestion for a minor improvement, about setting up the `EXCLUDE_LIST in a more module-specific way. The intent is that if we need to make module-specific changes to the exclude list in times going forward, we should do that in the module-specific test invocation, and not in the main module-independent code. test/jdk/tools/sincechecker/SinceChecker.java line 114: > 112: private static final Set EXCLUDE_LIST = Set.of( > 113: "java.lang.classfile" > 114: ); This should not really be here, in the general code for a SinceChecker for all modules. It would be better if this info was provided in command-line options, given when the test is run from the module-specific test (i.e. `CheckSince_javaBase.java`). This means you will have to write some additional simple option decoding in the `main` method, starting line 21. test/jdk/tools/sincechecker/modules/java_base/CheckSince_javaBase.java line 33: > 31: * jdk.compiler/com.sun.tools.javac.util > 32: * jdk.compiler/com.sun.tools.javac.code > 33: * @run main SinceChecker java.base See the comment about init-ing the EXCLUDE_LIST in the main SinceChecker, at about line 112. You are already paying a module name as an option to the main test. Maybe the command here on line 33 could be something like @run main SinceChecker java.base --exclude java.lang.classfile where the argument after `--exclude` is (logically) a comma-separated list of items to exclude. You could also do a whitespace-separated list up to the next option (i.e. item beginning with `-`) or end of the list. In other words make it future-proof against adding more options in future. - Changes requested by jjg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18934#pullrequestreview-2316850250 PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1767666131 PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r17676699
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v14]
On Thu, 19 Sep 2024 16:38:54 GMT, Nizar Benalla wrote: >> This checker checks the values of the `@since` tag found in the >> documentation comment for an element against the release in which the >> element first appeared. >> >> Real since value of an API element is computed as the oldest release in >> which the given API element was introduced. That is: >> - for modules, classes and interfaces, the release in which the element with >> the given qualified name was introduced >> - for constructors, the release in which the constructor with the given VM >> descriptor was introduced >> - for methods and fields, the release in which the given method or field >> with the given VM descriptor became a member of its enclosing class or >> interface, whether direct or inherited >> >> Effective since value of an API element is computed as follows: >> - if the given element has a `@since` tag in its javadoc, it is used >> - in all other cases, return the effective since value of the enclosing >> element >> >> The since checker verifies that for every API element, the real since value >> and the effective since value are the same, and reports an error if they are >> not. >> >> Preview method are handled as per JEP 12, if `@PreviewFeature` is used >> consistently going forward then the checker doesn't need to be updated with >> every release. The checker has explicit knowledge of preview elements that >> came before `JDK 14` because they weren't marked in a machine understandable >> way and preview elements that came before `JDK 17` that didn't have >> `@PreviewFeature`. >> >> Important note : We only check code written since `JDK 9` as the releases >> used to determine the expected value of `@since` tags are taken from the >> historical data built into `javac` which only goes back that far >> >> The intial comment at the beginning of `SinceChecker.java` holds more >> information into the program. >> >> I already have filed issues and fixed some wrong tags like in #18640, >> #18032, #18030, #18055, #18373, #18954, #18972. > > Nizar Benalla has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 28 commits: > > - Change "found" to "should be" in the error messages > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - avoid using `continue`, fix mistake in last commit > - extend SinceChecker to now skip some packages > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Merge remote-tracking branch 'upstream/master' into > source-based-since-checker > - Skip over files and packages that aren't found > - Move the tests to module/module_name directory for clearer naming > - (C) > - ... and 18 more: https://git.openjdk.org/jdk/compare/bc36ace7...d355222e For discussion: I see that in GitHub Actions, the test passes on all platforms. (That's good!). But it's not clear to me that it _needs_ to run on all platforms. Generally, the API should be the same on all platforms, right (After all, we only generate one set of API documentation.) So, the question becomes, can a test determine if it is being run in a multi-platform context? We presumably don't want to stop the test running whenever a user runs it locally on their platform of choice, but equally, if the test is being run on many platforms, that's an unnecessary use/waste of resources. But, I guess we run a lot of tests that are essentially platform-independent on all platforms, although I guess there're are also testing the product running on top of the JVM, whereas here. we're primarily just checking the source. Anyway, this is really just an abstract academic discussion, not a blocker in any way for the PR. - PR Comment: https://git.openjdk.org/jdk/pull/18934#issuecomment-2362333169
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v14]
On Thu, 19 Sep 2024 16:38:54 GMT, Nizar Benalla wrote:
>> This checker checks the values of the `@since` tag found in the
>> documentation comment for an element against the release in which the
>> element first appeared.
>>
>> Real since value of an API element is computed as the oldest release in
>> which the given API element was introduced. That is:
>> - for modules, classes and interfaces, the release in which the element with
>> the given qualified name was introduced
>> - for constructors, the release in which the constructor with the given VM
>> descriptor was introduced
>> - for methods and fields, the release in which the given method or field
>> with the given VM descriptor became a member of its enclosing class or
>> interface, whether direct or inherited
>>
>> Effective since value of an API element is computed as follows:
>> - if the given element has a `@since` tag in its javadoc, it is used
>> - in all other cases, return the effective since value of the enclosing
>> element
>>
>> The since checker verifies that for every API element, the real since value
>> and the effective since value are the same, and reports an error if they are
>> not.
>>
>> Preview method are handled as per JEP 12, if `@PreviewFeature` is used
>> consistently going forward then the checker doesn't need to be updated with
>> every release. The checker has explicit knowledge of preview elements that
>> came before `JDK 14` because they weren't marked in a machine understandable
>> way and preview elements that came before `JDK 17` that didn't have
>> `@PreviewFeature`.
>>
>> Important note : We only check code written since `JDK 9` as the releases
>> used to determine the expected value of `@since` tags are taken from the
>> historical data built into `javac` which only goes back that far
>>
>> The intial comment at the beginning of `SinceChecker.java` holds more
>> information into the program.
>>
>> I already have filed issues and fixed some wrong tags like in #18640,
>> #18032, #18030, #18055, #18373, #18954, #18972.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 28 commits:
>
> - Change "found" to "should be" in the error messages
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - avoid using `continue`, fix mistake in last commit
> - extend SinceChecker to now skip some packages
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - Skip over files and packages that aren't found
> - Move the tests to module/module_name directory for clearer naming
> - (C)
> - ... and 18 more: https://git.openjdk.org/jdk/compare/bc36ace7...d355222e
test/jdk/tools/sincechecker/SinceChecker.java line 50:
> 48: import jtreg.SkippedException;
> 49:
> 50: /*
Good comment.
test/jdk/tools/sincechecker/SinceChecker.java line 828:
> 826: }
> 827: if (version == null) {
> 828: //may be null for private elements
empty statement for `if`...
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1767663271
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1767662297
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module [v14]
On Thu, 19 Sep 2024 16:38:54 GMT, Nizar Benalla wrote:
>> This checker checks the values of the `@since` tag found in the
>> documentation comment for an element against the release in which the
>> element first appeared.
>>
>> Real since value of an API element is computed as the oldest release in
>> which the given API element was introduced. That is:
>> - for modules, classes and interfaces, the release in which the element with
>> the given qualified name was introduced
>> - for constructors, the release in which the constructor with the given VM
>> descriptor was introduced
>> - for methods and fields, the release in which the given method or field
>> with the given VM descriptor became a member of its enclosing class or
>> interface, whether direct or inherited
>>
>> Effective since value of an API element is computed as follows:
>> - if the given element has a `@since` tag in its javadoc, it is used
>> - in all other cases, return the effective since value of the enclosing
>> element
>>
>> The since checker verifies that for every API element, the real since value
>> and the effective since value are the same, and reports an error if they are
>> not.
>>
>> Preview method are handled as per JEP 12, if `@PreviewFeature` is used
>> consistently going forward then the checker doesn't need to be updated with
>> every release. The checker has explicit knowledge of preview elements that
>> came before `JDK 14` because they weren't marked in a machine understandable
>> way and preview elements that came before `JDK 17` that didn't have
>> `@PreviewFeature`.
>>
>> Important note : We only check code written since `JDK 9` as the releases
>> used to determine the expected value of `@since` tags are taken from the
>> historical data built into `javac` which only goes back that far
>>
>> The intial comment at the beginning of `SinceChecker.java` holds more
>> information into the program.
>>
>> I already have filed issues and fixed some wrong tags like in #18640,
>> #18032, #18030, #18055, #18373, #18954, #18972.
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 28 commits:
>
> - Change "found" to "should be" in the error messages
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - avoid using `continue`, fix mistake in last commit
> - extend SinceChecker to now skip some packages
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - Merge remote-tracking branch 'upstream/master' into
> source-based-since-checker
> - Skip over files and packages that aren't found
> - Move the tests to module/module_name directory for clearer naming
> - (C)
> - ... and 18 more: https://git.openjdk.org/jdk/compare/bc36ace7...d355222e
test/jdk/tools/sincechecker/SinceChecker.java line 152:
> 150: String version = String.valueOf(i);
> 151: Elements elements = ct.getElements();
> 152: elements.getModuleElement("java.base"); // forces module
> graph to be instantiated
@lahodaj I've always considered this kind of line to be a workaround; it would
be nice if we could have an RFE so this kind of line was unnecessary
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1767657515
Re: RFR: 8339789: Use index and definition tags in AnnotatedElement
On Mon, 9 Sep 2024 18:37:47 GMT, Joe Darcy wrote:
> Use index and definition tags in AnnotatedElement.
OK.
As a subsidiary follow-up, one might ask whether `{@index...}` should
implicitly include `` tags in its output.
-
Marked as reviewed by jjg (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20919#pullrequestreview-2290700260
Re: RFR: 8339631: Fix block @jls and @jvms tags [v2]
On Fri, 6 Sep 2024 12:09:16 GMT, Pavel Rappo wrote: > Thoughts? My initial thought is, at least we're working with specific tags (`@jls` and `@jvms`) rather than generic HTML links (`...`), since that permits the kind of detailed analysis and checking you are doing. Preview-ness is a pervasive feature of JDK these days, on the command-line and in APIs. Perhaps the tags here can be augmented to indicate that a feature is a preview feature, for example, `@jls preview 5.7.2 Unconditionally Exact Testing Conversions`. This could be used not only to affect the generated output, perhaps with PREVIEW, but could be used to help both determine the target for such links, and to review such links in subsequent releases. Such management should be part of the responsibilities of the preview-feature owner. - PR Comment: https://git.openjdk.org/jdk/pull/20879#issuecomment-2334271942
Re: RFR: 8339631: Fix block @jls and @jvms tags
On Thu, 5 Sep 2024 21:46:34 GMT, Pavel Rappo wrote:
> This fixes some of the recently discovered [issues] with the block variants
> of the specification tags. While reviewing, please check the proposed changes
> against the actual specifications. Since the specifications for JDK 23 are
> not yet available in the HTML format, you can use the specifications for JDK
> 22, which are reasonably up-to-date:
>
> - [JLS]
> - [JVMS]
>
> Note that this PR does NOT address any issues with the inline variants of the
> said tags. Those are harder to check. Even flagging suspicious tags requires
> a human. If you have some time, consider performing similar checks for inline
> `@jls` and `@jvms` tags in your area. Thanks.
>
> [issues]: https://bugs.openjdk.org/browse/JDK-8339558
> [JLS]: https://docs.oracle.com/javase/specs/jls/se22/html/index.html
> [JVMS]: https://docs.oracle.com/javase/specs/jvms/se22/html/index.html
possibly for later, a separate pass might be to review and make consistent the
use of `{@code }` in the tags, such as in `The ... Attribute`
-
Marked as reviewed by jjg (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/20879#pullrequestreview-2284150971
Re: RFR: 8339115: Rename TypeKind enum constants to follow code style
On Tue, 27 Aug 2024 22:15:17 GMT, Chen Liang wrote: > `TypeKind` enum constants are named in wrong code style; correct them before > finalization. > > Also improved `TypeKind` specification to talk about not mentioned > `returnType`, `void`, and subword types being erased to int (and how). See > the associated CSR too. > > See the HTML output for the changed `JSpec` taglet's `6.5.newarray` > rendering: > https://cr.openjdk.org/~liach/8339115-typekind/java.base/java/lang/classfile/TypeKind.html > > Note: when reviewing, please use > https://cr.openjdk.org/~iris/se/23/spec/draft/java-se-23-draft-spec-37/ > instead of JVMS 22 for reference; JVMS 23 has fixed a few ambiguities about > subword types being absent from JVM's stack and locals. Changes to JSpec.java look OK. No comment on other changes. - PR Review: https://git.openjdk.org/jdk/pull/20737#pullrequestreview-2264584894
Re: RFR: 8338014: Improve usage of @jvms tags in class file API
On Mon, 12 Aug 2024 18:42:25 GMT, Joe Darcy wrote: >> Hi all, >> >> This PR addresses [8338014](https://bugs.openjdk.org/browse/JDK-8338014) >> improving the use of `@jvms` tags by adding `JVMS` prior to the tag. >> >> Thanks, >> Sonia > > Looks fine; increasing review count to also include someone who more directly > maintains this API. > > If you haven't done so already, I recommend also doing a quick check for an > analagous issue with `@jls` tags in this API. @jddarcy Perhaps we should revisit the inline forms of `@jls` and `@jvms` tags to come up with similar and more consistent usage. - PR Comment: https://git.openjdk.org/jdk/pull/20513#issuecomment-2284691419
Re: RFR: 8336259: Wrong link to stylesheet.css in JavaDoc API documentation
On Thu, 11 Jul 2024 20:55:29 GMT, Nizar Benalla wrote: > Can I please get a review for this small change, the relative link to the > stylesheet isn't needed as it wasn't used anyway in the generated HTML. The > correct link to the stylesheet is already in the generated HTML. > > This is the difference between the generated docs before and after this > change, by running `diff -r` on the docs before and after the change. > > > GeneratedDocs % diff -r 8336259-stylesheet old-docs > diff -r 8336259-stylesheet/api/java.base/java/lang/doc-files/ValueBased.html > old-docs/api/java.base/java/lang/doc-files/ValueBased.html > 13a14 >> > title="Style"> > diff -r > 8336259-stylesheet/api/java.base/java/lang/doc-files/threadPrimitiveDeprecation.html > old-docs/api/java.base/java/lang/doc-files/threadPrimitiveDeprecation.html > 13a14 >> > title="Style"> > > > Here is the link for the generated docs after the change, no visual change is > seen but the erronous link is gone. > Value based classes > [before](https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/java.base/java/lang/doc-files/ValueBased.html) > - > [after](https://cr.openjdk.org/~nbenalla/GeneratedDocs/8336259-stylesheet/api/java.base/java/lang/doc-files/ValueBased.html) > > Java Thread Primitive Deprecation > [before](https://docs.oracle.com/en%2Fjava%2Fjavase%2F22%2Fdocs%2Fapi%2F%2F/java.base/java/lang/doc-files/threadPrimitiveDeprecation.html) > - > [after](https://cr.openjdk.org/~nbenalla/GeneratedDocs/8336259-stylesheet/api/java.base/java/lang/doc-files/threadPrimitiveDeprecation.html) Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/20145#pullrequestreview-2173773933
Re: RFR: 8332072: Convert package.html files in `java.naming` to package-info.java [v2]
On Wed, 26 Jun 2024 18:08:21 GMT, Nizar Benalla wrote:
>> Nizar Benalla has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - use javadoc standards
>> - Merge remote-tracking branch 'upstream/master' into naming
>> - remove whitespace
>> - 8332072: Convert package.html files in `java.naming` to package-info.java
>
> src/java.naming/share/classes/javax/naming/ldap/package-info.java line 200:
>
>> 198: * if (respCtls != null) {
>> 199: * // Find the one we want
>> 200: * for (int i = 0; i < respCtls.length; i++) {
>
> This is the only change that isn't a direct 1:1 conversion, I hope this ok.
This seems reasonable for what is an obvious but minor bug
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19529#discussion_r1655501908
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update [v2]
On Mon, 10 Jun 2024 21:38:18 GMT, Damon Nguyen wrote: >> While we do not translate Java keywords (when used as such, e.g. "`class` >> not allowed here"), and option names (e.g. `--class-path`) we have typically >> translated other text that is not mandated by the language or tools, (e.g. >> `the name of this class`, ``, ``. >> >> This change seems to be going backwards. >> >> Whatever the policy we use for translating (or not translating) parts of a >> message, we should be consistent across all tools and documents. > > @jonathan-gibbons I just realized that this change to the German file matches > the file with the Japanese and Chinese localized files. By that I mean the > "backwards" translations are also present for the Japanese and Chinese files. > > However, the previous versions of the Japanese and Chinese files don't have > these bits translated. So in summary, I can revert the German file back to > German but the Japanese and Chinese files will still be English for these > parts because I wouldn't know what to revert them to in their local language. > > As a result, I actually think it might be better to change this German file > as shown in the current version of the PR just to remain consistent with the > other localized versions of this file. We can still bring this up to the go > team as an issue, but I think this is the best solution now for this drop at > least. No one else except people reading this PR will spot inconsistencies between translations in different languages. So inconsistencies across languages will not be obvious to end users. But anyone (end-user) using the tool in any one of these languages is likely to spot inconsistencies in the translations for that one language. - PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633937355
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update
On Fri, 7 Jun 2024 22:46:44 GMT, Damon Nguyen wrote: > This issue is responsible for updating the translations of all the > localize(able) resources in the JDK. Primarily, the changes between JDK 22 > RDP 1 and the integration of the JDK 23 RDP 1 L10n drop will be translated. > > The translation tool adjusted some definitions, which causes some changes in > localized files where the source file had no changes. This causes some words > being reverted from localized languages to English, and some had its > definitions changed. > > Alternatively, the diffs are viewable here and was generated using Jonathan > Gibbons' diff tool for l10n: > https://cr.openjdk.org/~dnguyen/output2/ src/jdk.compiler/share/classes/com/sun/tools/javac/resources/compiler_de.properties line 409: > 407: compiler.err.unconditional.pattern.and.default=Switch umfasst sowohl ein > nicht bedingtes Muster als auch ein Standardlabel > 408: > 409: compiler.err.unconditional.pattern.and.both.boolean.values=Switch > umfasst sowohl boolesche Werte als auch ein nicht bedingtes Muster Here and elsewhere in this file and other translations, I'm surprised `switch` is not being translated, and is being capitalized. In the English form, it is not being used as a keyword, although could maybe be seen as a short form of `switch' statement, in which case `switch` should be treated as a keyword and not translated or capitalized. - PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633782242
Re: RFR: 8333827: JDK 23 RDP1 L10n resource files update
On Sun, 9 Jun 2024 22:07:44 GMT, Damon Nguyen wrote:
>> src/jdk.jshell/share/classes/jdk/internal/jshell/tool/resources/l10n_de.properties
>> line 183:
>>
>>> 181: jshell.fix.wrong.shortcut =Unerwartetes Zeichen nach
>>> Umschalt+Tab.\nVerwenden Sie "I" für automatischen Import, "V" zur
>>> Variablenerstellung oder "M" zur Methodenerstellung.\nWeitere Informationen
>>> finden Sie unter:\n/help shortcuts
>>> 182:
>>> 183: help.usage = Verwendung: jshell ... ...\nMögliche
>>> Optionen:\n--class-pathGibt an, wo die Benutzerklassendateien
>>> gespeichert sind\n--module-path Gibt an, wo die
>>> Anwendungsmodule gespeichert sind\n--add-modules (,)*\n
>>> Gibt aufzulösende Module oder alle Module im\n
>>>Modulpfad an, wenn ALL-MODULE-PATHs
>>> lautet\n--enable-native-access\n Ermöglicht
>>> Ausführung eingeschränkter nativer Methoden durch Code\n
>>> --enable-preview Code kann Vorschaufeatures in diesem Release nutzen\n
>>>--startup Ersetzung der Startdefinitionen mit einer
>>> Ausführung\n--no-startup Startdefinitionen werden nicht
>>> ausgeführt\n--feedback Gibt den anfänglichen Feedbackmodus
>>> an. Der Modus kann\nvordefiniert (Silent,
>>> Concise, Normal oder Verbose) oder\n
vorab benutzerdefiniert sein\n-qStilles Feedback.
Identisch mit: --feedback concise\n-sÄußerst stilles
Feedback. Identisch mit: --feedback silent\n-v
Verbose-Feedback. Identisch mit: --feedback verbose\n-J
Übergibt an das Laufzeitsystem, hat aber keine Auswirkungen\n
auf die Ausführung von Code-Snippets. Um Kennzeichen
anzugeben,\ndie die Ausführung von Code-Snippets
beeinflussen, verwenden Sie\n-R. Verwenden
Sie alternativ dazu -J mit\n--execution
local.\n-R Übergibt nur dann an das
Laufzeitsystem, wenn\nCode-Snippets ausgeführt
werden. Beispiel: -R-Dfoo=bar\nbedeutet, dass die
Ausführung des Snippets\nSystem.getProperty("foo")
"bar" zurück
gibt.\n-C Übergibt an den Java-Compiler in
JShell.\nBeispiel: -C-Xlint aktiviert alle
empfohlenen\n...
>
> This is what I've determined as well. I planned to leave these untranslated
> to reduce noise between drops as well.
While we do not translate Java keywords (when used as such, e.g. "`class` not
allowed here"), and option names (e.g. `--class-path`) we have typically
translated other text that is not mandated by the language or tools, (e.g. `the
name of this class`, ``, ``.
This change seems to be going backwards.
Whatever the policy we use for translating (or not translating) parts of a
message, we should be consistent across all tools and documents.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19609#discussion_r1633774912
Re: RFR: 8333828: Use value javadoc tag in java.lang.{Float, Double}
On Fri, 7 Jun 2024 21:05:33 GMT, Joe Darcy wrote:
> Use the value tag to make the javadoc for various format-related constants
> more informative to readers. Currently the information is available by
> following the "Constant Field Values" link.
>
> I'll reflow the paragraphs before a push.
You can omit the field name from a `{@value ...}` tag if the tag is in the doc
comment for that field. Thus, in most if not all the cases here, you can
simplify all tags to just `{@value}`.
-
PR Review: https://git.openjdk.org/jdk/pull/19607#pullrequestreview-2105442388
Re: RFR: 8330954: since-checker - Fix remaining `@since` tags in `java.base` [v4]
On Mon, 13 May 2024 20:14:38 GMT, Nizar Benalla wrote:
>> Please review this PR that aims to add all the remaining needed `@since`
>> tags in `java.base`, and group them into a single fix.
>> This is related to #18934 and my work around the `@since` checker feature.
>> Explicit `@since` tags are needed for some overriding methods for the
>> purpose of the checker.
>>
>> I will only give the example with the `CompletableFuture` class but here is
>> the before where the methods only appeared in "Methods declared in interface
>> N"
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/1749a355-133b-4a38-bffe-51ac415b2aac";>
>>
>>
>>
>> and after where the method has it's own javadoc, the main description is
>> added and the `@since` tags are added as intended.
>>
>> I don't have an account on `https://cr.openjdk.org/` but I could host the
>> generated docs somewhere if that is needed.
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/89b92288-9b5e-48ed-8fa1-9342ea43e043";>
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/9aef08ff-5030-4189-a996-582a7eef849b";>
>>
>> > src="https://github.com/openjdk/jdk/assets/96922791/fdd75a26-0396-4c5e-8f59-a3717b7d7ec8";>
>>
>>
>> TIA
>
> Nizar Benalla has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains 11 additional
> commits since the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into 8330954
> - (C)
> - (C)
> - Move security classes changes to pr18373
> - remove couple extra lines
> - Pull request is now only about `@since` tags, might add an other commit
> - add one more `{inheritDoc}` to `CompletableFuture.state`
> - add `@throws` declarations to javadoc
> - add ``{@inheritDoc}`` to new javadoc comments
> - remove two extra spaces
> - ... and 1 more: https://git.openjdk.org/jdk/compare/39767aaa...b3574b97
I disagree somewhat with the statements in the comments that the checker should
follow the javadoc rules, whatever they are.
The important thing is to decide what the rules for `@since` should be, in
terms of changes to the set of signatures in the class. Generally, I think the
rule should be that _every_ declaration should have `@since` _except_ that
members need not have the tag if it would be the same as for the enclosing
class or interface. As far as adding an overriding method is concerned, if it
has the same VM descriptor as the overridden method, it is not a "new" method
in the class; if it has a covariant return type, that is a significant change
to the descriptor and so such methods should have `@since`.
As a practical rule for deciding whether any declaration is new or not, imagine
writing a test program that refers to the most specific form of the
declaration. If that test program does not compile on JDK version N-1 and does
compile on version N, then it warrants having `@since N`. Put another way,
`@since N` should identify the first release in which the declaration can be
used in the given form.
Note, these rules are stated without reference to what `javadoc` does.
`javadoc` should follow these rules as well; it is a bug if the tool generates
incorrect documentation based on `@since` tags following these rules.
Also, while the proposed new Since Checker should follow these rules when
analysing declarations, it may go further when making suggestions to correct
errors that it finds. For example, instead of simply saying _No `@since` tag
found here_, it might analyze the history and say _No `@since` tag found here;
the declaration was introduced in X_ for an appropriate X.
-
PR Comment: https://git.openjdk.org/jdk/pull/18954#issuecomment-2145605004
Integrated: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 7 May 2024 22:23:48 GMT, Jonathan Gibbons wrote: > With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). This pull request has now been integrated. Changeset: 10eb1cb6 Author:Jonathan Gibbons URL: https://git.openjdk.org/jdk/commit/10eb1cb639095caa2636cc87c45201d4f8cf1eb4 Stats: 117 lines in 25 files changed: 0 ins; 0 del; 117 mod 8331879: Clean up non-standard use of /// comments in `java.base` Reviewed-by: naoto, iris, darcy - PR: https://git.openjdk.org/jdk/pull/19130
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base` [v2]
> With the advent of JEP 467, `///` comments may be treated as documentation > comments, and may be subject to the recently new `javac` warning about > "dangling doc comments" in unexpected places. > > In keeping with the policy to keep the `java.base` module free of all `javac` > warnings, this patch proposes edits to existing uses of `///`. > > There are two dominant policies in the proposed changes. > 1. A long horizontal line of `/` is replaced by `//-` > 2. A long vertical series of lines beginning `///` is replaced by lines > beginning `//|`. > > As with all style changes, I have also tried to honor local usage, for > consistency. > > In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, > `CLOVER:OFF`). I investigated the use of such comments to determine that the > exact form of the comment prefix was not significant. (Phew!) > > > (This PR is informally blocked by JEP 467). Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: incorporate review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19130/files - new: https://git.openjdk.org/jdk/pull/19130/files/3e039b43..e77a4d14 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19130&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19130&range=00-01 Stats: 24 lines in 7 files changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/19130.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130 PR: https://git.openjdk.org/jdk/pull/19130
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Tue, 28 May 2024 20:01:46 GMT, Alan Bateman wrote: > > OK. I was just trying to honor the apparent intent to make the comment > > stand out more than just a plain `//` comment, but I have no strong > > feelings against reducing `///` to `//` > > In this case I would reduce it to '//' but others will have different > opinions of course. What about changing `///` to `//---` to give slightly more prominence to these comments, over plain old `//` comments. The dashes give a small sense of a horizontal rule, to delimit sections of code. (FWIW, I have locally edited `//|` to `//` and such comments do not stand out beside existing use of `//`.) - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2136042843
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 23 May 2024 05:52:57 GMT, Alan Bateman wrote: > > A long vertical series of lines beginning /// is replaced by lines > > beginning //|. > > This one looks unusual when it's just one line, I could imagine deleting the > "|" in these cases. OK. I was just trying to honor the apparent intent to make the comment stand out more than just a plain `//` comment, but I have no strong feelings against reducing `///` to `//` - PR Comment: https://git.openjdk.org/jdk/pull/19130#issuecomment-2135914838
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Wed, 22 May 2024 20:13:08 GMT, Naoto Sato wrote: >> With the advent of JEP 467, `///` comments may be treated as documentation >> comments, and may be subject to the recently new `javac` warning about >> "dangling doc comments" in unexpected places. >> >> In keeping with the policy to keep the `java.base` module free of all >> `javac` warnings, this patch proposes edits to existing uses of `///`. >> >> There are two dominant policies in the proposed changes. >> 1. A long horizontal line of `/` is replaced by `//-` >> 2. A long vertical series of lines beginning `///` is replaced by lines >> beginning `//|`. >> >> As with all style changes, I have also tried to honor local usage, for >> consistency. >> >> In one place, a pair of comments appeared to contain directives >> (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to >> determine that the exact form of the comment prefix was not significant. >> (Phew!) >> >> >> (This PR is informally blocked by JEP 467). > > src/java.base/share/classes/jdk/internal/icu/impl/StringPrepDataReader.java > line 122: > >> 120: * see store.c of gennorm for more information and values >> 121: */ >> 122: // /* dataFormat="SPRP" 0x53, 0x50, 0x52, 0x50 */ > > This source file is coming from the upstream ICU4J project. Even if this is a > `non-standard` comment, I would keep it as it is to minimize the merge effort. As a non-standard comment, it will trigger a warning, since the prevailing standard for `java.base` is to compile with all warnings enabled and no warnings found. - PR Review Comment: https://git.openjdk.org/jdk/pull/19130#discussion_r1617755455
RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
With the advent of JEP 467, `///` comments may be treated as documentation comments, and may be subject to the recently new `javac` warning about "dangling doc comments" in unexpected places. In keeping with the policy to keep the `java.base` module free of all `javac` warnings, this patch proposes edits to existing uses of `///`. There are two dominant policies in the proposed changes. 1. A long horizontal line of `/` is replaced by `//-` 2. A long vertical series of lines beginning `///` is replaced by lines beginning `//|`. As with all style changes, I have also tried to honor local usage, for consistency. In one place, a pair of comments appeared to contain directives (`CLOVER:ON`, `CLOVER:OFF`). I investigated the use of such comments to determine that the exact form of the comment prefix was not significant. (Phew!) (This PR is informally blocked by JEP 467). - Commit messages: - incorporate review comments - Merge remote-tracking branch 'upstream/master' into 8330177.dangling-java.base - JDK-8331879: Clean up non-standard use of /// comments in `java.base` Changes: https://git.openjdk.org/jdk/pull/19130/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19130&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331879 Stats: 117 lines in 25 files changed: 0 ins; 0 del; 117 mod Patch: https://git.openjdk.org/jdk/pull/19130.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19130/head:pull/19130 PR: https://git.openjdk.org/jdk/pull/19130
Re: RFR: 8332109: Convert remaining tests using com.sun.tools.classfile to ClassFile API
On Sun, 12 May 2024 08:36:44 GMT, Chen Liang wrote: > Some tests are not migrated to the ClassFile API in previous migrations. > > - Some are simple oversights that didn't remove usages of > com.sun.tools.classfile; > - The CallerSensitive ones used an old utility, replaced by CF API-based new > code; > - many in javac are because the files are compiled with older source > compatibility. Those patches are converted to have the source code stored in > text blocks and compiled within tests using `ToolBox#writeJavaFiles` and > `CompilerUtils.compile`; > - As described in the JBS issue, there are a few other tests not covered; > one is in #19193 while the others are blocked by CreateSymbols migration or > bugs. > > Testing: all modified tests pass. javadoc test change looks OK. - PR Review: https://git.openjdk.org/jdk/pull/19206#pullrequestreview-2055992753
Re: RFR: 8331879: Clean up non-standard use of /// comments in `java.base`
On Thu, 9 May 2024 02:09:50 GMT, xiaotaonan wrote: > Clean up non-standard use of /// comments in `java.base` This PR is premature. Until JEP 467 is integrated, there is nothing special about `///` comments, and the compiler does not report on non-standard use. There is a Draft PR for this issue ready to go once JEP 467 has been integrated, that addresses all necessary comments in `java.base`. https://github.com/openjdk/jdk/pull/19130 - PR Comment: https://git.openjdk.org/jdk/pull/19151#issuecomment-2104836545
Re: RFR: 8305457: Implement java.io.IO [v5]
On Thu, 9 May 2024 14:23:37 GMT, Pavel Rappo wrote: >> Please review this PR which introduces the `java.io.IO` top-level class and >> three methods to `java.io.Console` for [Implicitly Declared Classes and >> Instance Main Methods (Third Preview)]. >> >> This PR has been obtained as `git merge --squash` of a now obsolete [draft >> PR]. >> >> [Implicitly Declared Classes and Instance Main Methods (Third Preview)]: >> https://bugs.openjdk.org/browse/JDK-8323335 >> [draft PR]: https://github.com/openjdk/jdk/pull/18921 > > Pavel Rappo has updated the pull request incrementally with five additional > commits since the last revision: > > - Simplify output.exp > - Cover null prompt in input tests > - Make input test parametric > - Specify behaviour in regard to null > - Add explicit @throws IOError to IO methods (Minor). at least within the context of this PR, tags like `@param` and `@throws` are stylistically inconsistent as to whether they begin with a capital letter and end with a period. (I have not checked for local consistency within the affected files.) test/jdk/java/io/IO/IO.java line 125: > 123: * To simulate a terminal, the test currently uses the EXPECT(1) Unix > 124: * command, which does not work for Windows. Later, a library like > pty4j > 125: * or JPty might be used instead EXPECT, to cover both Unix and > Windows. Possible missing word `of`: `instead of EXPECT` - PR Review: https://git.openjdk.org/jdk/pull/19112#pullrequestreview-2048123137 PR Review Comment: https://git.openjdk.org/jdk/pull/19112#discussion_r1595543569
Re: RFR: 8330205: Initial troff manpage generation for JDK 24
On Tue, 7 May 2024 11:53:19 GMT, Pavel Rappo wrote: > Please review this mechanical change to man pages. This PR should be > integrated after https://github.com/openjdk/jdk/pull/18787. Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/19119#pullrequestreview-2044314419
Re: RFR: 8330954: Fix remaining `@since` tags in `java.base`
While `@since` might not be considered a normative part of the specification, (it's effectively a cache of derived meta-data) it is part of the generated documentation, and as such deserves to be correct. -- Jon On 5/5/24 4:33 PM, Pavel Rappo wrote: On Mon, 29 Apr 2024 17:46:24 GMT, Nizar Benalla wrote: Pavel, can I simply change the PR/issue title to be more descriptive? As I want to include the the inherit doc because of the unchecked exceptions, rather than clean this up in a different PR I suggest dropping all the changes that are irrelevant to `@since` from this PR, unless you have PRs with similar mixed changes integrated. As far as I know, `@since` is not a normative part of documentation, whereas `@throws` is. So the latter is a bigger change and would require more scrutiny and more area experts during review. In the future, we might want to have yet another aid/tool: a "`@throws` checker" that finds all overriding methods that do not declare some of the unchecked exceptions that the methods they override do. - PR Review Comment: https://git.openjdk.org/jdk/pull/18954#discussion_r1583618873
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module
On Mon, 29 Apr 2024 11:31:23 GMT, Nizar Benalla wrote:
>> test/jdk/tools/sincechecker/SinceChecker.java line 423:
>>
>>> 421: }
>>> 422:
>>> 423: //these were preview in before the introduction of the
>>> @PreviewFeature
>>
>> Just curious: where do you find this information?
>
> Used a script to get all the elements with `@Deprecated(forRemoval=true,
> since=...)`, `@jdk.internal.preview`, `{@preview Associated with ` in
> their javadoc.
> And then had to cross reference with a document that had all the ids
Thanks for the update
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18934#discussion_r1586827124
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module
On Wed, 24 Apr 2024 14:10:29 GMT, Nizar Benalla wrote:
> This checker checks the values of the `@since` tag found in the documentation
> comment for an element against the release in which the element first
> appeared.
>
> Real since value of an API element is computed as the oldest release in which
> the given API element was introduced. That is:
> - for modules, classes and interfaces, the release in which the element with
> the given qualified name was introduced
> - for constructors, the release in which the constructor with the given VM
> descriptor was introduced
> - for methods and fields, the release in which the given method or field with
> the given VM descriptor became a member of its enclosing class or interface,
> whether direct or inherited
>
> Effective since value of an API element is computed as follows:
> - if the given element has a `@since` tag in its javadoc, it is used
> - in all other cases, return the effective since value of the enclosing
> element
>
> The since checker verifies that for every API element, the real since value
> and the effective since value are the same, and reports an error if they are
> not.
>
> Preview method are handled as per JEP 12, if `@PreviewFeature` is used
> consistently going forward then the checker doesn't need to be updated with
> every release. The checker has explicit knowledge of preview elements that
> came before `JDK 14` because they weren't marked in a machine understandable
> way and preview elements that came before `JDK 17` that didn't have
> `@PreviewFeature`.
>
> Important note : We only check code written since `JDK 9` as the releases
> used to determine the expected value of `@since` tags are taken from the
> historical data built into `javac` which only goes back that far
>
> The intial comment at the beginning of `SinceChecker.java` holds more
> information into the program.
>
> I already have filed issues and fixed some wrong tags like in #18640, #18032,
> #18030, #18055, #18373, #18954, #18972.
test/jdk/TEST.groups line 669:
> 667: # Set of tests for `@since` checks in source code documentation
> 668: jdk_since_check = \
> 669:tools/sincechecker/testjavabase/CheckSince_javaBase.java
Generally, the symbol of a red line in a red circle is indicative of a missing
final newline at the end of the file.
It is recommend that files end with exactly one newline character.
test/jdk/tools/sincechecker/SinceChecker.java line 53:
> 51: - This checker checks the values of the `@since` tag found in the
> documentation comment for an element against the release in which the element
> first appeared.
> 52: - The source code containing the documentation comments is read from
> `src.zip` in the release of JDK used to run the test.
> 53: - The releases used to determine the expected value of `@since` tags are
> taken from the historical data built into `javac`.
(Minor, suggest wrapping lines somewhere between 80-100 characters long)
test/jdk/tools/sincechecker/SinceChecker.java line 72:
> 70:
> 71: Real since value of an API element is computed as the oldest release in
> which the given API element was introduced. That is:
> 72: - for modules, classes and interfaces, the release in which the element
> with the given qualified name was introduced
What about packages? packages should be in this list as well.
test/jdk/tools/sincechecker/SinceChecker.java line 223:
> 221: try (FileSystem zipFO = FileSystems.newFileSystem(uri,
> Collections.emptyMap())) {
> 222: Path root = zipFO.getRootDirectories().iterator().next();
> 223: Path packagePath = root.resolve(moduleName);
Here, and in uses of this name, including in method signatures, it is strange
to see the variable named `packagePath` since it appears to be the path of the
unnamed package in the module.
`modulePath` would likely be a better name, unless you are worried about
possible confusion with the `--module-path` option, in which case maybe
`moduleDir` or `moduleDirectory` would work.
test/jdk/tools/sincechecker/SinceChecker.java line 239:
> 237: } catch (Exception e) {
> 238: e.printStackTrace();
> 239: error("Initiating javadocHelperFailed " +
> e.getMessage());
You probably want just `e`, not `e.getMessage()` because `e` (actually
`e.toString()`) will include the name of the exception as well, whereas
`e.getMessage()` does not.)
test/jdk/tools/sincechecker/SinceChecker.java line 249:
> 247: }
> 248:
> 249: private void processModuleCheck(ModuleElement moduleElement,
> JavacTask ct, Path packagePath, EffectiveSourceSinceHelper javadocHelper) {
see comment line 223, about use of `packagePath`
test/jdk/tools/sincechecker/SinceChecker.java line 280:
> 278: }
> 279:
> 280: private String getModuleVersionFromFile(Path packagePath) {
see comment line 223, about `packagePath`
test/jdk/tools/sincechecker/Si
Re: RFR: 8331077 : nroff man page update for jar tool [v2]
On Wed, 1 May 2024 18:42:12 GMT, Weibing Xiao wrote: >> nroff man page update for jar tool. >> This update is caused by the change of >> https://bugs.openjdk.org/browse/JDK-8318971. While the .md man pages got >> updated in other repos, the corresponding nroff man page was never updated >> in OpenJDK repos > > Weibing Xiao has updated the pull request incrementally with one additional > commit since the last revision: > > version of java Approved, as corresponding to the upstream Markdown file. That being said, the descriptions about arg files in lines 334-343 look somewhat clunky, and could benefit from some improvements in a later update. - Marked as reviewed by jjg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19039#pullrequestreview-2034355345
Re: RFR: 8331077 : nroff man page update for jar tool
On Wed, 1 May 2024 17:52:13 GMT, Weibing Xiao wrote: > nroff man page update for jar tool. > This update is caused by the change of > https://bugs.openjdk.org/browse/JDK-8318971. While the .md man pages got > updated in other repos, the corresponding nroff man page was never updated in > OpenJDK repos Changes requested by jjg (Reviewer). src/jdk.jartool/share/man/jar.1 line 38: > 36: . ftr VBI CBI > 37: .\} > 38: .TH "JAR" "1" "2024" "JDK 23-internal" "JDK Commands" should remain as `ea` -- not `internal` - PR Review: https://git.openjdk.org/jdk/pull/19039#pullrequestreview-2034148426 PR Review Comment: https://git.openjdk.org/jdk/pull/19039#discussion_r1586584726
Re: RFR: 8331051: Add an `@since` checker test for `java.base` module
On Wed, 24 Apr 2024 14:10:29 GMT, Nizar Benalla wrote:
> This checker checks the values of the `@since` tag found in the documentation
> comment for an element against the release in which the element first
> appeared.
>
> Real since value of an API element is computed as the oldest release in which
> the given API element was introduced. That is:
> - for modules, classes and interfaces, the release in which the element with
> the given qualified name was introduced
> - for constructors, the release in which the constructor with the given VM
> descriptor was introduced
> - for methods and fields, the release in which the given method or field with
> the given VM descriptor became a member of its enclosing class or interface,
> whether direct or inherited
>
> Effective since value of an API element is computed as follows:
> - if the given element has a `@since` tag in its javadoc, it is used
> - in all other cases, return the effective since value of the enclosing
> element
>
> The since checker verifies that for every API element, the real since value
> and the effective since value are the same, and reports an error if they are
> not.
>
> Important note : We only check code written since `JDK 9` as the releases
> used to determine the expected value of `@since` tags are taken from the
> historical data built into `javac` which only goes back that far
>
> The intial comment at the beginning of `SinceChecker.java` holds more
> information into the program.
>
> I already have filed issues and fixed some wrong tags like in #18640, #18032,
> #18030, #18055, #18373, #18954, #18972.
Various comments inline. Overall, a great start.
Consider adding more explanatory comments for the bigger items, like classes
and long/important methods. Imagine a future-person looking over your shoulder,
asking what the more important items do or are for.
test/jdk/tools/sincechecker/SinceChecker.java line 49:
> 47: import java.util.regex.Matcher;
> 48: import java.util.regex.Pattern;
> 49: import java.util.stream.Collectors;
The imports are somewhat unordered. I recommend the follooing order
* `java.*` imports first,
* then `javax.*` imports
* then `com.sun.*` imports
* and finally the `jtreg` import.
And, alphabetically sorted in each group.
test/jdk/tools/sincechecker/SinceChecker.java line 52:
> 50:
> 51: /*
> 52: The `@since` checker works as a two-step process:
Insert an initial sentence or paragraph, such as the following:
This checker checks the values of the `@since` tag found in the documentation
comment for an element
against the release in which the element first appeared.
The source code containing the documentation comments is read from `src.zip` in
the release of
JDK used to run the test. The releases used to determine the expected value of
`@since` tags
are taken from the historical data built into `javac`.
test/jdk/tools/sincechecker/SinceChecker.java line 79:
> 77: - When an element is still marked as preview, the `@since` should be the
> first JDK release where the element was added.
> 78: - If the element is no longer marked as preview, the `@since` should be
> the first JDK release where it was no longer preview.
> 79:
Add a comment about need for special treatment of early preview features.
test/jdk/tools/sincechecker/SinceChecker.java line 87:
> 85: public class SinceChecker {
> 86: private final Map> LEGACY_PREVIEW_METHODS = new
> HashMap<>();
> 87: private final Map classDictionary = new
> HashMap<>();
See comment lower down about `class IntroducedIn`
test/jdk/tools/sincechecker/SinceChecker.java line 116:
> 114: }
> 115:
> 116: private void processModuleRecord(ModuleElement moduleElement, String
> releaseVersion, JavacTask ct) {
for this method, and similarly named methods, the use of `Record` seems
confusing, if only because there do not seem to be any record classes being
processed or analyzed. Consider dropping `Record` or changing it to
`Element`. Same for `analyzePackageRecord`, `analyzeClassRecord`.
Also, consider being consistent with `process` or analyze`.
test/jdk/tools/sincechecker/SinceChecker.java line 135:
> 133: private void analyzeClassRecord(TypeElement te, String version,
> Types types, Elements elements) {
> 134: Set classModifiers = te.getModifiers();
> 135: if (!(classModifiers.contains(Modifier.PUBLIC) ||
> classModifiers.contains(Modifier.PROTECTED))) {
Insert comment:
/// JDK documentation only contains public and protected declarations
which is 99% true. (The serialization page can contain `private`
declarations...)
test/jdk/tools/sincechecker/SinceChecker.java line 140:
> 138: persistElement(te.getEnclosingElement(), te, types, version);
> 139: elements.getAllMembers(te).stream()
> 140: .filter(element ->
> element.getModifiers().contains(Modifier.PUBLIC) ||
> element.getModifiers().contains(Modifier.PROTECTED))
Consider writing and usin
Integrated: 8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:04:30 GMT, Jonathan Gibbons wrote:
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
This pull request has now been integrated.
Changeset: a920af23
Author:Jonathan Gibbons
URL:
https://git.openjdk.org/jdk/commit/a920af233a11592af113f456f7608cb59dd1617e
Stats: 482 lines in 58 files changed: 385 ins; 3 del; 94 mod
8303689: javac -Xlint could/should report on "dangling" doc comments
Reviewed-by: vromero, ihse, prr
-
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v10]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
fix typo
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/39689a52..48e8b0a2
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=09
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=08-09
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v9]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
revert need to disable warning
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/16a265a2..39689a52
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=08
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=07-08
Stats: 3 lines in 1 file changed: 0 ins; 2 del; 1 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v8]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 11 commits:
- Merge remote-tracking branch 'upstream/master' into
8303689.dangling-comments # Please enter a commit message to explain why this
merge is necessary, # especially if it merges an updated upstream into a topic
branch. # # Lines starting with '#' will be ignored, and an empty message
aborts # the
commit.
- Merge with upstream/master
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- ... and 1 more: https://git.openjdk.org/jdk/compare/1c238d43...16a265a2
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=07
Stats: 485 lines in 59 files changed: 387 ins; 3 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v7]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains 10 commits:
- Merge with upstream/master
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=06
Stats: 485 lines in 59 files changed: 387 ins; 3 del; 95 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Integrated: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote: > Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. This pull request has now been integrated. Changeset: 9cc163a9 Author:Jonathan Gibbons URL: https://git.openjdk.org/jdk/commit/9cc163a999eb8e9597d45b095b642c25071043bd Stats: 134 lines in 23 files changed: 50 ins; 56 del; 28 mod 8330178: Clean up non-standard use of /** comments in `java.base` Reviewed-by: darcy, iris, dfuchs, aivanov, naoto - PR: https://git.openjdk.org/jdk/pull/18846
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Fri, 19 Apr 2024 19:29:31 GMT, Alexey Ivanov wrote: > > We do not have an overall style guide. The conventional wisdom for editing > > any existing file is to follow the style in that file, if such a style can > > be discerned. > > That's what I do. > > I saw either style used in JDK. Yet I didn't check which style is more > common… to decide which style I should use when creating new files with > javadoc comments. [How to Write Doc Comments for the Javadoc > Tool](https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html) > doesn't have a blank line between the javadoc comment and class declaration; > nor do javadoc comments for fields and methods. The document [How to Write Doc Comments for the Javadoc Tool](https://www.oracle.com/uk/technical-resources/articles/java/javadoc-tool.html) is depressingly obsolete, as indicated by this text towards the end: >Footnotes > > [1] At Java Software, we use @version for the SCCS version. See "man > sccs-get" for details. The consensus seems to be the following: - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2070379608
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 20:38:05 GMT, Naoto Sato wrote: > OK, fair enough. Approving for the `icu` part Thank you. - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2067280359
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 19:47:20 GMT, Naoto Sato wrote: > Unless it is absolutely necessary, I would not fix comments in > `jdk.internal.icu` sources, as they are in the upstream code, and would like > to minimize the merging effort. @naotoj Given the policy and strong desire to compile `java.base` with all lint warnings enabled and `-Werror`, all of the proposed changes in this PR will each individually break the build if/when the warning is added to `javac` and we continue to compile `java.base` with the current build settings. - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2067246948
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Fri, 19 Apr 2024 11:32:55 GMT, Pavel Rappo wrote: > This comment is not a review. I simply use the opportunity provided by this > PR to suggest that we stop making new `/** ... */` and separately fix old > jtreg comments like this: > > ``` > /** > * @test TestSmallHeap > * @bug 8067438 8152239 > * @summary Verify that starting the VM with a small heap works > * @library /test/lib > * @modules java.base/jdk.internal.misc > * @build jdk.test.whitebox.WhiteBox > * @run driver jdk.test.lib.helpers.ClassFileInstaller > jdk.test.whitebox.WhiteBox > * @run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions > -XX:+WhiteBoxAPI gc.TestSmallHeap > */ > ``` > > I know that those comments only appear in tests, and that tests are never > documented. Still, it is confusing to see such comments and IDEs might frown > upon them too. Generally, it is a good rule of thumb that `/** ... */` should > be reserved only for javadoc. I agree we should not be using `/**` for test description comments. IntelliJ tells me there are 103+ matches in 97+ files, so this would be a non-trivial cleanup. I note there are 22 issues in `test/langtools/jdk` tests (i.e. javadoc tests), so let's start there ;-) - PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2067164929
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
> Please review a set of updates to clean up use of `/**` comments in the > vicinity of declarations. > > There are various categories of update: > > * "Box comments" beginning with `/**` > * Misplaced doc comments before package or import statements > * Misplaced doc comments after the annotations for a declaration > * Dangling `/**` comments relating to a group of declarations, separate from > the doc comments for each of those declarations > * Use of `/**` for the legal header at or near the top of the file > > In one case, two doc comments before a declaration were merged, which fixes a > bug/omission in the documented serialized form. Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java Fix grammatical typo Co-authored-by: Alexey Ivanov - Changes: - all: https://git.openjdk.org/jdk/pull/18846/files - new: https://git.openjdk.org/jdk/pull/18846/files/fcbe02d5..d7b46a85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=18846&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=18846&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18846.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18846/head:pull/18846 PR: https://git.openjdk.org/jdk/pull/18846
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base` [v2]
On Fri, 19 Apr 2024 10:49:11 GMT, Alexey Ivanov wrote: >> Jonathan Gibbons has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update >> src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java >> >> Fix grammatical typo >> >> Co-authored-by: Alexey Ivanov > > src/java.base/share/classes/sun/net/www/protocol/file/FileURLConnection.java > line 38: > >> 36: >> 37: /** >> 38: * Open an file input stream given a URL. > > Suggestion: > > * Open a file input stream given a URL. OK, I'll accept this as a minor typo mixup, that does not in itself need a CSR. But generally, it is not a goal to fix issues in the content of doc comments. - PR Review Comment: https://git.openjdk.org/jdk/pull/18846#discussion_r1572821973
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Thu, 18 Apr 2024 20:44:00 GMT, Jonathan Gibbons wrote:
> Please review a set of updates to clean up use of `/**` comments in the
> vicinity of declarations.
>
> There are various categories of update:
>
> * "Box comments" beginning with `/**`
> * Misplaced doc comments before package or import statements
> * Misplaced doc comments after the annotations for a declaration
> * Dangling `/**` comments relating to a group of declarations, separate from
> the doc comments for each of those declarations
> * Use of `/**` for the legal header at or near the top of the file
>
> In one case, two doc comments before a declaration were merged, which fixes a
> bug/omission in the documented serialized form.
> It is somewhat off-topic. Yet I noticed javadoc comments in some files are
> followed a blank line, and others aren't. Out of my curiosity, is there a
> convention about the blank line between the javadoc comment and the class or
> interface declaration? Should there be one?
>
> ```java
> /**
> * This is a class.
> */
> public class A {}
> ```
>
> or
>
> ```java
> /**
> * This is a class.
> */
>
> public class A {}
> ```
>
> Which is the most common? Which is preferred?
We do not have an overall style guide. The conventional wisdom for editing
any existing file is to follow the style in that file, if such a style can be
discerned.
-
PR Comment: https://git.openjdk.org/jdk/pull/18846#issuecomment-2067156407
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Fri, 19 Apr 2024 10:44:27 GMT, Alexey Ivanov wrote: >> Please review a set of updates to clean up use of `/**` comments in the >> vicinity of declarations. >> >> There are various categories of update: >> >> * "Box comments" beginning with `/**` >> * Misplaced doc comments before package or import statements >> * Misplaced doc comments after the annotations for a declaration >> * Dangling `/**` comments relating to a group of declarations, separate from >> the doc comments for each of those declarations >> * Use of `/**` for the legal header at or near the top of the file >> >> In one case, two doc comments before a declaration were merged, which fixes >> a bug/omission in the documented serialized form. > > src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 37: > >> 35: import static sun.security.util.SecurityProviderConstants.*; >> 36: >> 37: /* > > Should this be included in the main javadoc comment? The `@author` tags > belong in the javadoc comment. > > The javadoc itself is likely unreadable, when processed the entire comment > becomes one very long paragraph. That would be up to the relevant component to decide how to improve these comments. The goal here is proactively fix any warnings that may be generated about multiple doc comments on a declaration. I note that neither comment contributes to the public API, and that `@author` tags are generally obsolete these days. The VCS metadata is a more accurate reflection of individual contributions. - PR Review Comment: https://git.openjdk.org/jdk/pull/18846#discussion_r1572818078
Re: RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
On Fri, 19 Apr 2024 10:53:11 GMT, Alexey Ivanov wrote:
>> Please review a set of updates to clean up use of `/**` comments in the
>> vicinity of declarations.
>>
>> There are various categories of update:
>>
>> * "Box comments" beginning with `/**`
>> * Misplaced doc comments before package or import statements
>> * Misplaced doc comments after the annotations for a declaration
>> * Dangling `/**` comments relating to a group of declarations, separate from
>> the doc comments for each of those declarations
>> * Use of `/**` for the legal header at or near the top of the file
>>
>> In one case, two doc comments before a declaration were merged, which fixes
>> a bug/omission in the documented serialized form.
>
> src/java.base/macosx/classes/apple/security/AppleProvider.java line 33:
>
>> 31: /*
>> 32: * The Apple Security Provider.
>> 33: */
>
> Does it make sense to incorporate this comment into the following javadoc
> comment?
>
>
> /**
> * Defines the (an) Apple security provider.
> * ...
> */
> @SuppressWarnings("serial") // JDK implementation class
Maybe, but only maybe, and if so, it would be out of scope for this work. That
would be up to the relevant component team.
That being said, neither comment is part of any public API, and this comment is
effectively already present in the first sentence of the actual doc comment.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18846#discussion_r1572813320
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v6]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The pull request now contains seven commits:
- Merge with upstream/master
- update test
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=05
Stats: 488 lines in 61 files changed: 389 ins; 3 del; 96 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v5]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
update test
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/f3670e7a..8ad8b818
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=03-04
Stats: 6 lines in 1 file changed: 6 ins; 0 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v4]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request with a new target base due to a
merge or a rebase. The incremental webrev excludes the unrelated changes
brought in by the merge/rebase. The pull request contains five additional
commits since the last revision:
- improve handling of ignorable doc comments
suppress warning when building test code
- Merge remote-tracking branch 'upstream/master' into 8303689.dangling-comments
- call `saveDanglingDocComments` for local variable declarations
- adjust call for `saveDanglingDocComments` for enum members
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/3f745431..f3670e7a
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=02-03
Stats: 42074 lines in 1058 files changed: 18282 ins; 15937 del; 7855 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
RFR: 8330178: Clean up non-standard use of /** comments in `java.base`
Please review a set of updates to clean up use of `/**` comments in the vicinity of declarations. There are various categories of update: * "Box comments" beginning with `/**` * Misplaced doc comments before package or import statements * Misplaced doc comments after the annotations for a declaration * Dangling `/**` comments relating to a group of declarations, separate from the doc comments for each of those declarations * Use of `/**` for the legal header at or near the top of the file In one case, two doc comments before a declaration were merged, which fixes a bug/omission in the documented serialized form. - Commit messages: - JDK-8330178: Clean up non-standard use of /** comments in `java.base` Changes: https://git.openjdk.org/jdk/pull/18846/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18846&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8330178 Stats: 134 lines in 23 files changed: 50 ins; 56 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/18846.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18846/head:pull/18846 PR: https://git.openjdk.org/jdk/pull/18846
Re: RFR: 8330458: Add missing @since tag to ClassFile.JAVA_23_VERSION
On Wed, 17 Apr 2024 20:46:26 GMT, Joe Darcy wrote: > 8330458: Add missing @since tag to ClassFile.JAVA_23_VERSION Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18828#pullrequestreview-2007300284
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v3]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
call `saveDanglingDocComments` for local variable declarations
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/56d6dcac..3f745431
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=01-02
Stats: 10 lines in 1 file changed: 5 ins; 2 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v2]
On Wed, 3 Apr 2024 10:01:37 GMT, Magnus Ihse Bursie wrote: >> Jonathan Gibbons has updated the pull request incrementally with one >> additional commit since the last revision: >> >> adjust call for `saveDanglingDocComments` for enum members > > The build changes look okay. > > Do you have any plan of going through all the Java modules and fixing the > issues, or opening JBS issues to have them fixed? Or will these lint warnings > remain disabled for the foreseeable future? @magicus > The build changes look okay. > > Do you have any plan of going through all the Java modules and fixing the > issues, or opening JBS issues to have them fixed? Or will these lint warnings > remain disabled for the foreseeable future? The plan is to create an umbrella bug to clean up the individual modules. There is interest to clean up `java.base`, to keep that one free of any warnings, and I can see that the lang tools modules will get cleaner up as well. It will be up to other component teams to decide if and when to clean up other parts of the system. Once this work has been integrated, it is relatively easy to enable the warnings for a module and to fix the ensuing issues. Since any changes "only" involve comments, it should be reasonably easy to fix them, unlike some pervasive other warnings, like `this-escape`. - PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2052174696
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v2]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
adjust call for `saveDanglingDocComments` for enum members
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/3d6f1f95..56d6dcac
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=00-01
Stats: 5 lines in 1 file changed: 3 ins; 2 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
On Wed, 27 Mar 2024 22:41:33 GMT, Pavel Rappo wrote: > Would this be the first lint -- not doclint -- warning related to comments, > let alone doc comments? No. `-Xlint:dep-ann` correlates `@Deprecated` annotations with `@deprecated` tags in doc comments. > src/jdk.javadoc/share/man/javadoc.1 line 111: > >> 109: source code with the \f[V]javac\f[R] option \f[V]-Xlint\f[R], or more >> 110: specifically, \f[V]-Xlint:dangling-doc-comments\f[R]. >> 111: Within a source file, you may use suppress any warnings generated by > > Typo? > Suggestion: > > Within a source file, you may suppress any warnings generated by Thanks; I'll check the underlying original. - PR Comment: https://git.openjdk.org/jdk/pull/18527#issuecomment-2024162355 PR Review Comment: https://git.openjdk.org/jdk/pull/18527#discussion_r1542157047
RFR: JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
Please review the updates to support a proposed new
`-Xlint:dangling-doc-comments` option.
The work can be thought of as in 3 parts:
1. An update to the `javac` internal class `DeferredLintHandler` so that it is
possible to specify the appropriately configured `Lint` object when it is time
to consider whether to generate the diagnostic or not.
2. Updates to the `javac` front end to record "dangling docs comments" found
near the beginning of a declaration, and to report them using an instance of
`DeferredLintHandler`. This allows the warnings to be enabled or disabled using
the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The procedure
for handling dangling doc comments is described in this comment in
`JavacParser`.
* Dangling documentation comments are handled as follows.
* 1. {@code Scanner} adds all doc comments to a queue of
* recent doc comments. The queue is flushed whenever
* it is known that the recent doc comments should be
* ignored and should not cause any warnings.
* 2. The primary documentation comment is the one obtained
* from the first token of any declaration.
* (using {@code token.getDocComment()}.
* 3. At the end of the "signature" of the declaration
* (that is, before any initialization or body for the
* declaration) any other "recent" comments are saved
* in a map using the primary comment as a key,
* using this method, {@code saveDanglingComments}.
* 4. When the tree node for the declaration is finally
* available, and the primary comment, if any,
* is "attached", (in {@link #attach}) any related
* dangling comments are also attached to the tree node
* by registering them using the {@link #deferredLintHandler}.
* 5. (Later) Warnings may be genereated for the dangling
* comments, subject to the {@code -Xlint} and
* {@code @SuppressWarnings}.
3. Updates to the make files to disable the warnings in modules for which the
warning is generated. This is often because of the confusing use of `/**` to
create box or other standout comments.
-
Commit messages:
- JDK-8303689: javac -Xlint could/should report on "dangling" doc comments
Changes: https://git.openjdk.org/jdk/pull/18527/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=18527&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8303689
Stats: 477 lines in 60 files changed: 368 ins; 5 del; 104 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
Re: RFR: 8297879: javadoc link to preview JEP 1000 has grouping character comma
On Mon, 18 Mar 2024 14:53:44 GMT, Pavel Rappo wrote: > Please review this simple bugfix to properly construct links to preview JEPs. > > The most straightforward fix I could think of was to pass `String` rather > than `int` (`Integer`) to a method, which eventually calls > `java.text.MessageFormat.format(String, Object...)`. > > For the test, I decided to be ~lazy~ practical and piggyback on the existing > infrastructure. The alternatives were: > > 1. slap `noreg-hard` on the JBS bug and skip testing > 2. create a sophisticated test that dynamically adds a constant into the > `PreviewFeature.Feature` enum, annotates some class with `PreviewFeature` > with that constant, and finally documents that class with `PreviewFeature` > patched into `java.base` > > While (1) is insufficient, (2) seems overkill in this case. Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18350#pullrequestreview-1943374124
Re: RFR: JDK-8323760 putIfAbsent documentation conflicts with itself [v2]
On Tue, 13 Feb 2024 17:11:05 GMT, John Hendrikx wrote:
>> Update the documentation for `@return` tag of `putIfAbsent` to match the
>> main description. `putIfAbsent` uses the same wording as `put` for its
>> `@return` tag, but that is incorrect. `putIfAbsent` never returns the
>> **previous** value, as the whole point of the method is not the replace the
>> value if it was present. As such, if it returns a value, it is the
>> **current** value, and in all other cases it will return `null`.
>
> John Hendrikx has updated the pull request incrementally with four additional
> commits since the last revision:
>
> - Add code block around argument
> - Reword
> - Reword to use put's previous value wording
> - Reword more clearly
src/java.base/share/classes/java/util/Map.java line 824:
> 822: * {@code null} return can also indicate that the map
> previously
> 823: * associated {@code null} with the key, if the
> implementation supports
> 824: * null values.)
inconsistent typography for `null`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17438#discussion_r1489648755
Re: RFR: 8322041: JDK 22 RDP1 L10n resource files update [v2]
On Thu, 14 Dec 2023 22:17:54 GMT, Alisen Chung wrote: >> Translation drop for JDK22 RDP1 > > Alisen Chung has updated the pull request incrementally with one additional > commit since the last revision: > > removed quotes around Ablehnen The diffs are more conveniently available here: https://cr.openjdk.org/~jjg/jdk-l10n-diffs--15dec23/ The compiler and javadoc diffs look "ok" but I cannot read most other languages, and cannot comment on the quality of the translations. I do see that there are places where a translation has been edited where the English original has not. I also see issues with some of the English messages, but that is not for here. My "approval" is just for `jdk.compiler` and `jdk.javadoc` files. - Marked as reviewed by jjg (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17096#pullrequestreview-1785076089
Re: RFR: JDK-8315458 Implement JEP 463: Implicitly Declared Classes and Instance Main Method (Second Preview) [v31]
On Tue, 21 Nov 2023 18:51:55 GMT, Jim Laskey wrote: > The comments are attached to the modifiers (first thing it encounters.) I’m > sure the javadoc toolset has a method that gets the comments from the right > thing. In the AST created by the parser, doc comments should be attached to _declarations_ (`JCClassDecl`, `JCMethodDecl`, `JCVariableDecl` etc) not modifiers. There is nothing in the javadoc toolset that _gets the comments from the right thing_. - PR Comment: https://git.openjdk.org/jdk/pull/16461#issuecomment-1821645764
Re: RFR: 8318027: Support alternative name to jdk.internal.vm.compiler [v3]
On Fri, 20 Oct 2023 15:45:50 GMT, Doug Simon wrote: >> The Graal code base has >> [renamed](https://github.com/oracle/graal/commit/1e41203d10db321f86723eac90f6cd0573b08b33) >> its module to `jdk.compiler.graal` as part of preparations for Project >> Galahad. Due to the way Java modules work, this requires a JDK change. The >> core of the issue is that the >> [service](https://github.com/openjdk/jdk/blob/56aa1e8dc8047cbc29d554889c64beb6eca0b8eb/src/jdk.internal.vm.ci/share/classes/module-info.java#L37) >> by which HotSpot requests a Graal compilation is defined in JVMCI. Since >> JVMCI is in the boot layer, the service can only be implemented by a >> provider in the boot layer and the package defining the service must be >> exported to the provider's defining module. This export currently targets >> [`jdk.internal.vm.compiler`](https://github.com/openjdk/jdk/blob/56aa1e8dc8047cbc29d554889c64beb6eca0b8eb/src/jdk.internal.vm.ci/share/classes/module-info.java#L28) >> and so the binding fails for the new Graal module. To address this, this PR >> reflects the Graal module ren aming, including adjusting the qualified export. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > re-fix since tags to reflect current JDK release The proposed name `jdk.compiler.graal` makes it seem like the module is strongly related to the `jdk.compiler` module, which contains `javac` and related tools, whereas these modules are completely unrelated. Is there not a better name that can be used? - PR Comment: https://git.openjdk.org/jdk/pull/16189#issuecomment-1773211363
Re: RFR: 8308753: Class-File API transition to Preview [v2]
On Tue, 26 Sep 2023 12:32:37 GMT, Adam Sotona wrote: >> Classfile API is an internal library under package `jdk.internal.classfile` >> in JDK 21. >> This pull request turns the Classfile API into a preview feature and moves >> it into `java.lang.classfile`. >> It repackages all uses across JDK and tests and adds lots of missing Javadoc. >> >> This PR goes in sync with >> [JDK-8308754](https://bugs.openjdk.org/browse/JDK-8308754): Class-File API >> (Preview) (CSR) >> and [JDK-8280389](https://bugs.openjdk.org/browse/JDK-8280389): Class-File >> API (Preview) (JEP). >> >> Online javadoc is available at: >> https://cr.openjdk.org/~asotona/JDK-8308753-preview/api/java.base/java/lang/classfile/package-summary.html >> >> In addition to the primary transition to preview, this pull request also >> includes: >> - All Classfile* classes ranamed to ClassFile* (based on JEP discussion). >> - A new preview feature, `CLASSFILE_API`, has been added. >> - Buildsystem tool required a little patch to support annotated modules. >> - All JDK modules using the Classfile API are newly participating in the >> preview. >> - All tests that use the Classfile API now have preview enabled. >> - A few Javac tests not allowing preview have been partially reverted; their >> conversion can be re-applied when the Classfile API leaves preview mode. >> >> Despite the number of affected files, this pull request is relatively >> straight-forward. The preview version of the Classfile API is based on the >> internal version of the library from the JDK master branch, and there are no >> API features added. >> >> Please review this pull request to help the Classfile API turn into a >> preview. >> >> Any comments are welcome. >> >> Thanks, >> Adam > > Adam Sotona has updated the pull request incrementally with one additional > commit since the last revision: > > PreviewFeature annotated with JEP 457 I don't see anything specific to `javadoc` - PR Review: https://git.openjdk.org/jdk/pull/15706#pullrequestreview-1649221047
Re: RFR: 8267174: Many test files have the wrong Copyright header
On Tue, 5 Sep 2023 22:49:41 GMT, Erik Joelsson wrote: > There are a number of files in the `test` directory that have an incorrect > copyright header, which includes the "classpath" exception text. This patch > removes that text from all test files that I could find it in. I did this > using a combination of `sed` and `grep`. Reviewing this patch is probably > easier using the raw patch file or a suitable webrev format. > > It's my assumption that these headers were introduced by mistake as it's > quite easy to copy the wrong template when creating new files. One has to wonder about the `**/*_OLD.java` files, but that would be a different cleanup - PR Review: https://git.openjdk.org/jdk/pull/15573#pullrequestreview-1612069262
Integrated: JDK-8310909: java.io.InvalidObjectException has redundant `@since` tag
On Mon, 26 Jun 2023 18:32:30 GMT, Jonathan Gibbons wrote: > Please review a trivial update to remove a redundant `@since` tag. This pull request has now been integrated. Changeset: 46add3f8 Author: Jonathan Gibbons URL: https://git.openjdk.org/jdk/commit/46add3f8e3ea5d08130e0342390f998979c2a14e Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod 8310909: java.io.InvalidObjectException has redundant `@since` tag Reviewed-by: lancea, naoto, bpb, darcy, iris - PR: https://git.openjdk.org/jdk/pull/14662
RFR: JDK-8310909: java.io.InvalidObjectException has redundant `@since` tag
Please review a trivial update to remove a redundant `@since` tag. - Commit messages: - JDK-8310909: java.io.InvalidObjectException has redundant `@since` tag Changes: https://git.openjdk.org/jdk/pull/14662/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14662&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8310909 Stats: 2 lines in 1 file changed: 0 ins; 2 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/14662.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14662/head:pull/14662 PR: https://git.openjdk.org/jdk/pull/14662
Re: [jdk21] RFR: 8309670: java -help output for --module-path / -p is incomplete
On Fri, 23 Jun 2023 11:32:04 GMT, Christian Stein wrote: > Hi all, > > This pull request contains a backport of commit > [4bf78162](https://github.com/openjdk/jdk/commit/4bf78162c52564645af79b8324b69d89102dc024) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Christian Stein on 23 Jun 2023 > and was reviewed by Mandy Chung and Alan Bateman. > > Thanks! Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk21/pull/57#pullrequestreview-1499007038
Re: RFR: 8309632: JDK 21 RDP1 L10n resource files update [v2]
On Tue, 13 Jun 2023 18:38:28 GMT, Naoto Sato wrote: > Left some comments on the translations mainly in Japanese. It is now very > easy to look at the l10n changes in the generated HTML. One small comment to > the tool is that it would be nice if the order in HTML (alphabetically sorted > currently) aligns with the order in the actual resource file. It is a great > improvement anyways. 1. It is not easy to recover the order in the actual resource file: the contents are read into a `Properties` object which uses a `Hashtable` to store the data. 2. There is no guarantee that the order is the same in all the resource files, so we would have to choose one, presumably the default one, to determine the presentation order. - PR Comment: https://git.openjdk.org/jdk/pull/14430#issuecomment-1594844435
Integrated: JDK-8309686: inconsistent URL for https://www.unicode.org/reports/tr35
On Thu, 8 Jun 2023 21:36:03 GMT, Jonathan Gibbons wrote: > Please review a trivial docs change to fix a URL in a `@spec` tag consistent > with equivalent URLs in other tags. > (Consistency will be required when the External Specifications page is > enabled.) This pull request has now been integrated. Changeset: 6f492e80 Author: Jonathan Gibbons URL: https://git.openjdk.org/jdk/commit/6f492e800597c9ce332b9d5b54c00f551f145a0d Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8309686: inconsistent URL for https://www.unicode.org/reports/tr35 Reviewed-by: naoto - PR: https://git.openjdk.org/jdk/pull/14384
RFR: JDK-8309686: inconsistent URL for https://www.unicode.org/reports/tr35
Please review a trivial docs change to fix a URL in a `@spec` tag consistent with equivalent URLs in other tags. (Consistency will be required when the External Specifications page is enabled.) - Commit messages: - JDK-8309686: inconsistent URL for https://www.unicode.org/reports/tr35 Changes: https://git.openjdk.org/jdk/pull/14384/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14384&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8309686 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/14384.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14384/head:pull/14384 PR: https://git.openjdk.org/jdk/pull/14384
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 21:37:42 GMT, Jonathan Gibbons wrote: >> Sure we can; this relates to an earlier comment of yours on >> Utils.isDirectSupertype here: >> https://github.com/openjdk/jdk/pull/14357#discussion_r1222053011 > > The general criticism here is whether we should restrict in any way the set > of super types from which one can inherit documentation, and/or what should > the set of checks be? > > For example, if a method in C inherits a method in B that has no comment but > which inherits a method with a comment in A, then it seems unduly restrictive > to stop the method in C explicitly referencing the method in A, as compared > to only allowing a reference to B, which implicitly gets its comment from A. > > That being said, there is merit in starting off with restrictions and > loosening them in the face of experience, rather than trying to increase the > stylistic restrictions later. > Sure we can; this relates to an earlier comment of yours on > Utils.isDirectSupertype here: [#14357 > (comment)](https://github.com/openjdk/jdk/pull/14357#discussion_r1222053011) Thanks for tying these together; I had not realized they were related. - PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r103243
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
Great work in a difficult area overflowing with Technical Debt.
I've made various (optional) suggestions, I'll approve for now, but will look
again if you want to make ore changes.
-
Marked as reviewed by jjg (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/14357#pullrequestreview-1468628610
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 20:48:58 GMT, Pavel Rappo wrote: >> test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java >> line 673: >> >>> 671: * >>> 672: * For now a warning is issued if a doc comment inherits from >>> 673: * an indirect supertype. >> >> Not sure I agree with this. Can we discuss? > > Sure we can; this relates to an earlier comment of yours on > Utils.isDirectSupertype here: > https://github.com/openjdk/jdk/pull/14357#discussion_r1222053011 The general criticism here is whether we should restrict in any way the set of super types from which one can inherit documentation, and/or what should the set of checks be? For example, if a method in C inherits a method in B that has no comment but which inherits a method with a comment in A, then it seems unduly restrictive to stop the method in C explicitly referencing the method in A, as compared to only allowing a reference to B, which implicitly gets its comment from A. That being said, there is merit in starting off with restrictions and loosening them in the face of experience, rather than trying to increase the stylistic restrictions later. - PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r102085
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
line 75:
> 73: // code is OK, it likely isn't (after all, there's an unknown
> tag).
> 74: setAutomaticCheckNoStacktrace(true);
> 75: { // DocLint is on
"on" -> "explicit"
test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
line 85:
> 83: }
> 84: }
> 85: // DocLint is off
"off" -> "default"
test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
line 289:
> 287:
> 288: /*
> 289: * C1.m inherits documentation from B1.m explicitly and undirect.
possible typo: do you mean "indirect"
test/langtools/jdk/javadoc/doclet/testDirectedInheritance/TestDirectedInheritance.java
line 673:
> 671: *
> 672: * For now a warning is issued if a doc comment inherits from
> 673: * an indirect supertype.
Not sure I agree with this. Can we discuss?
test/langtools/jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java
line 61:
> 59: import toolbox.ToolBox;
> 60:
> 61: /*
Great description! :-)
test/langtools/jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java
line 111:
> 109: *built.
> 110: */
> 111: public class TestMethodCommentsAlgorithm extends JavadocTester {
General comment for entire class: great/clever test!
test/langtools/jdk/javadoc/doclet/testMethodCommentAlgorithm/TestMethodCommentsAlgorithm.java
line 273:
> 271: }
> 272: }
> 273: System.err.println("Test suite root: " + p);
You might want to use `JavadocTester.out` instead of `System.err`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222117577
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222117867
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222119420
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222122630
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222124323
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222130052
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222126057
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
line 2847:
> 2845: break;
> 2846: }
> 2847: if (isPlainInterface(peek) && !isPublic(peek) &&
> !isLinkable(peek)) {
What is the significance of the `isPublic` check?
I'm developing a knee-jerk reaction to such checks, in the face of
access-related command-line options.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222055578
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/DocFinder.java
line 54:
> 52:
> 53: @SuppressWarnings("serial")
> 54: public static final class NoOverriddenMethodFound extends Exception {
General comment, for future work:
Maybe we can combine/merge/unify this exception with `ThrowsTaglet.Failure`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/util/Utils.java
line 671:
> 669: //.anyMatch(t -> Objects.equals(typeUtils.asElement(t),
> typeUtils.asElement(t2)));
> 670:
> 671: return true; /* disabled for causing issues in JDK API
> Documentation build */
Please describe the issues and/or provide a JBS issue, so that we can decide
when to revert the code to the commented-out form.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222051685
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222053011
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritableTaglet.java
line 53:
> 51: *
> 52: * In the future, this could be reworked using some other mechanism,
> 53: * such as throwing an exception.
Should `ThrowsTaglet.Failure` (eventually?) be declared here in
`InheritableTaglet`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222046447
Re: RFR: 8285368: Overhaul doc-comment inheritance [v2]
On Wed, 7 Jun 2023 16:02:40 GMT, Pavel Rappo wrote:
>> Please review this long-awaited change to documentation inheritance.
>>
>> This change improves "methods comment algorithm" and introduces directed
>> documentation inheritance. While "methods comment algorithm" -- automatic
>> search for inheritable documentation -- has been improved, it still cannot
>> read an author's mind so as to always find the documentation they intended.
>> From now on, an author can state their intention, by providing an FQN of the
>> superclass or superinterface from which to inherit documentation:
>>
>> {@inheritDoc S}
>>
>> Which is exactly what I did to counterbalance some of the JDK API
>> Documentation changes caused by the change to "methods comment algorithm".
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> feedback: make warning less scary
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
line 94:
> 92: if (supertype == null) {
> 93: messages.error(inheritDocPath,
> "doclet.inheritDocBadSupertype");
> 94: return replacement;
`replacement` may be empty.
Consider using `HtmlDocletWriter.invalidTagOutput`.
(Later, we might want to provide a utility message to wrap `messages.error` and
`invalidTagOutput`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
line 110:
> 108: for (Element e : methods) {
> 109: ExecutableElement m = (ExecutableElement) e;
> 110: if (configuration.utils.elementUtils.overrides(method,
> m, (TypeElement) method.getEnclosingElement())) {
Suggestion for eventual future cleanup (not this PR): reduce need for chained
fields like`config.utils.elementUtils`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
line 110:
> 108: for (Element e : methods) {
> 109: ExecutableElement m = (ExecutableElement) e;
> 110: if (configuration.utils.elementUtils.overrides(method,
> m, (TypeElement) method.getEnclosingElement())) {
Note that `configuration.utils` is available as `utils`
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/InheritDocTaglet.java
line 120:
> 118: // that this method overrides
> 119: messages.error(inheritDocPath,
> "doclet.inheritDocBadSupertype");
> 120: return replacement;
Another case for `invalidTagOutput` ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222037366
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222032679
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222035009
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1222037884
Re: RFR: 8285368: Overhaul doc-comment inheritance
On Wed, 7 Jun 2023 14:19:16 GMT, Pavel Rappo wrote:
> Please review this long-awaited change to documentation inheritance.
>
> This change improves "methods comment algorithm" and introduces directed
> documentation inheritance. While "methods comment algorithm" -- automatic
> search for inheritable documentation -- has been improved, it still cannot
> read an author's mind so as to always find the documentation they intended.
> From now on, an author can state their intention, by providing an FQN of the
> superclass or superinterface from which to inherit documentation:
>
> {@inheritDoc S}
>
> Which is exactly what I did to counterbalance some of the JDK API
> Documentation changes caused by the change to "methods comment algorithm".
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1325:
> 1323: //
> 1324: // m.at(pos).newInheritDocTree(ref)
> 1325: //
Although the scary warning is not wrong, I think it would have been enough to
say something more concise, like "_use the original legacy method if no ref is
given_"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/14357#discussion_r1221797159
Re: RFR: JDK-8306584: Start of release updates for JDK 22
On Thu, 20 Apr 2023 20:28:18 GMT, Joe Darcy wrote: > Time to get JDK 22 underway... test/langtools/tools/javac/versions/Versions.java line 26: > 24: /* > 25: * @test > 26: * @bug 4981566 5028634 5094412 6304984 7025786 7025789 8001112 8028545 > 8000961 8030610 8028546 8188870 8173382 8173382 8193290 8205619 8028563 > 8245147 8245586 8257453 8286035 8306586 At some point, this line should be wrapped test/langtools/tools/javac/versions/Versions.java line 93: > 91: TWENTY(false,"64.0", "20", Versions::checksrc20), > 92: TWENTY_ONE(false,"65.0", "21", Versions::checksrc20), > 93: TWENTY_TWO(false,"66.0", "22", Versions::checksrc20); when do these get updated? - PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1202974585 PR Review Comment: https://git.openjdk.org/jdk/pull/13567#discussion_r1202976605
Re: RFR: JDK-8304036: Use CommandLine class from shared module [v2]
On Fri, 21 Apr 2023 21:14:00 GMT, Christian Stein wrote: >> This pull request addresses the open ends left by >> [JDK-8236919](https://bugs.openjdk.org/browse/JDK-8236919): >> - #11272 >> >> Changes: >> - [x] Extend list of targeted exports of `jdk.internal.opt/jdk.internal.opt` >> to `jdk.compiler` and `jdk.javadoc` >> - [x] Use shared `CommandLine.java` in `jdk.compiler` module >> - [x] Use shared `CommandLine.java` in `jdk.javadoc` module >> - [x] Remove `CommandLine.java` from `jdk.compiler` module > > Christian Stein has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains three additional > commits since the last revision: > > - Merge branch 'openjdk:master' into JDK-8304036-reusable-command-line-part-2 > - Add missing import > - JDK-8304036: Use CommandLine class from shared module Marked as reviewed by jjg (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/12997#pullrequestreview-1396422793
Re: RFR: JDK-8304036: Use CommandLine class from shared module
On Mon, 13 Mar 2023 08:16:54 GMT, Christian Stein wrote: > This pull request addresses the open ends left by > [JDK-8236919](https://bugs.openjdk.org/browse/JDK-8236919): > - #11272 > > Changes: > - [x] Extend list of targeted exports of `jdk.internal.opt/jdk.internal.opt` > to `jdk.compiler` and `jdk.javadoc` > - [x] Use shared `CommandLine.java` in `jdk.compiler` module > - [x] Use shared `CommandLine.java` in `jdk.javadoc` module > - [x] Remove `CommandLine.java` from `jdk.compiler` module Looks good, for when un-Draft-ed - PR Comment: https://git.openjdk.org/jdk/pull/12997#issuecomment-1487214895
Re: RFR: 8304896: Update to use jtreg 7.2
On Tue, 18 Apr 2023 12:08:50 GMT, Andrey Turbanov wrote: > Interesting, why this JBS ticked is considered as a bug? There's no obvious best choice here (bug, enhancement, task) and as is, it was the same as for similar previous items. - PR Comment: https://git.openjdk.org/jdk/pull/13496#issuecomment-1513340471
Re: RFR: JDK-8305713: DocCommentParser: merge blockContent and inlineContent [v3]
> Please review a cleanup in DocCommentParser to merge blockContent and > inlineContent into a single method to parse "rich content" in a doc comment. Jonathan Gibbons has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 59 commits: - Merge branch 'pr/13362' into pr/13362 - Merge remote-tracking branch 'upstream/master' into 8305713.dcp-content - 8305809: (fs) Review obsolete Linux kernel dependency on os.version (Unix kernel 2.6.39) Reviewed-by: rriggs, alanb - 8294806: jpackaged-app ignores splash screen from jar file Reviewed-by: almatvee - 8305368: G1 remset chunk claiming may use relaxed memory ordering Reviewed-by: ayang, iwalulya - 8305370: Inconsistent use of for_young_only_phase parameter in G1 predictions Reviewed-by: iwalulya, kbarrett - 8305663: Wrong iteration order of pause array in g1MMUTracker Reviewed-by: ayang, tschatzl - 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries Reviewed-by: alanb, kevinw - 8305419: JDK-8301995 broke building libgraal Reviewed-by: matsaave, dnsimon, thartmann - 8302696: Revert API signature changes made in JDK-8285504 and JDK-8285263 Reviewed-by: mullan - ... and 49 more: https://git.openjdk.org/jdk/compare/5501bf21...45ee028b - Changes: https://git.openjdk.org/jdk/pull/13431/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13431&range=02 Stats: 15292 lines in 500 files changed: 7710 ins; 6358 del; 1224 mod Patch: https://git.openjdk.org/jdk/pull/13431.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13431/head:pull/13431 PR: https://git.openjdk.org/jdk/pull/13431
Re: RFR: JDK-8305713: DocCommentParser: merge blockContent and inlineContent [v2]
> Please review a cleanup in DocCommentParser to merge blockContent and > inlineContent into a single method to parse "rich content" in a doc comment. Jonathan Gibbons has updated the pull request incrementally with 42 additional commits since the last revision: - Merge remote-tracking branch 'upstream/master' into 8305713.dcp-content - 8305809: (fs) Review obsolete Linux kernel dependency on os.version (Unix kernel 2.6.39) Reviewed-by: rriggs, alanb - 8294806: jpackaged-app ignores splash screen from jar file Reviewed-by: almatvee - 8305368: G1 remset chunk claiming may use relaxed memory ordering Reviewed-by: ayang, iwalulya - 8305370: Inconsistent use of for_young_only_phase parameter in G1 predictions Reviewed-by: iwalulya, kbarrett - 8305663: Wrong iteration order of pause array in g1MMUTracker Reviewed-by: ayang, tschatzl - 8305761: Resolve multiple definition of 'jvm' when statically linking with JDK native libraries Reviewed-by: alanb, kevinw - 8305419: JDK-8301995 broke building libgraal Reviewed-by: matsaave, dnsimon, thartmann - 8302696: Revert API signature changes made in JDK-8285504 and JDK-8285263 Reviewed-by: mullan - 8304738: UnregisteredClassesTable_lock never created Reviewed-by: iklam, jcking, dholmes - ... and 32 more: https://git.openjdk.org/jdk/compare/b8b43eae...ab56c463 - Changes: - all: https://git.openjdk.org/jdk/pull/13431/files - new: https://git.openjdk.org/jdk/pull/13431/files/b8b43eae..ab56c463 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13431&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13431&range=00-01 Stats: 9487 lines in 384 files changed: 2654 ins; 5914 del; 919 mod Patch: https://git.openjdk.org/jdk/pull/13431.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13431/head:pull/13431 PR: https://git.openjdk.org/jdk/pull/13431
RFR: JDK-8305713: DocCommentParser: merge blockContent and inlineContent
Please review a cleanup in DocCommentParser to merge blockContent and inlineContent into a single method to parse "rich content" in a doc comment. - Depends on: https://git.openjdk.org/jdk/pull/13362 Commit messages: - JDK-8305713: DocCommentParser: merge blockContent and inlineContent - 8272119: Typo in JDK documentation (a -> an) - 8305461: [vectorapi] Add VectorMask::xor - 8305608: Change VMConnection to use "test.class.path"instead of "test.classes" - 8274166: Some CDS tests ignore -Dtest.cds.runtime.options - 8304745: Lazily initialize byte[] in java.io.BufferedInputStream - 8267140: Support closing the HttpClient by making it auto-closable - 8269843: typo in LinkedHashMap::removeEldestEntry spec - 8305480: test/hotspot/jtreg/runtime/NMT/VirtualAllocCommitMerge.java failing on 32 bit arm - 8305607: Remove some unused test parameters in com/sun/jdi tests - ... and 6 more: https://git.openjdk.org/jdk/compare/44f33ad1...b8b43eae Changes: https://git.openjdk.org/jdk/pull/13431/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13431&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305713 Stats: 5934 lines in 119 files changed: 5071 ins; 480 del; 383 mod Patch: https://git.openjdk.org/jdk/pull/13431.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13431/head:pull/13431 PR: https://git.openjdk.org/jdk/pull/13431
Integrated: JDK-8305206: Add @spec tags in java.base/java.* (part 1)
On Thu, 30 Mar 2023 17:24:11 GMT, Jonathan Gibbons wrote: > Please review a change to add `@spec` tags (and remove some equivalent `@see` > tags) to the main "core-libs" packages in `java.base` module. > > This is similar to, and a subset of, PR #11073. That PR was withdrawn, and > based on the ensuing discussion and suggestion, is now being handled with a > series of PRs for various separate parts of the system. Follow-up PRs will > be provided for the rest of `java.base`, for `java.desktop`, and for XML > APIs. The "LangTools" modules have already been updated. The "External > Specifications" page has been temporarily [disabled][] until this work is > complete. > > While the primary content of the change was automated, I've manually adjusted > the formatting, to break long lines. > > It is clear there is significant inconsistency in the ordering of block tags > in doc comment. We might want to (separately) consider normalizing the > order of the tags, perhaps according to the order defined for the tags in the > generated output, as given [here][] > > [here]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 > [disabled]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 This pull request has now been integrated. Changeset: c6bd489c Author:Jonathan Gibbons URL: https://git.openjdk.org/jdk/commit/c6bd489cc8d30fb6eec865b3dab1cf861e25c8d7 Stats: 270 lines in 60 files changed: 268 ins; 2 del; 0 mod 8305206: Add @spec tags in java.base/java.* (part 1) Reviewed-by: alanb, naoto, darcy, lancea, dfuchs, iris, mchung - PR: https://git.openjdk.org/jdk/pull/13248
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1) [v2]
On Fri, 31 Mar 2023 16:28:14 GMT, Sean Mullan wrote: > I didn't see any changes to security APIs - are they coming in a follow-on > issue? Yes, this is _Add `@spec` tags in java.base/java.* (part 1)_ The rest of `java.base` will be in part 2. - PR Comment: https://git.openjdk.org/jdk/pull/13248#issuecomment-1492670942
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1) [v3]
> Please review a change to add `@spec` tags (and remove some equivalent `@see` > tags) to the main "core-libs" packages in `java.base` module. > > This is similar to, and a subset of, PR #11073. That PR was withdrawn, and > based on the ensuing discussion and suggestion, is now being handled with a > series of PRs for various separate parts of the system. Follow-up PRs will > be provided for the rest of `java.base`, for `java.desktop`, and for XML > APIs. The "LangTools" modules have already been updated. The "External > Specifications" page has been temporarily [disabled][] until this work is > complete. > > While the primary content of the change was automated, I've manually adjusted > the formatting, to break long lines. > > It is clear there is significant inconsistency in the ordering of block tags > in doc comment. We might want to (separately) consider normalizing the > order of the tags, perhaps according to the order defined for the tags in the > generated output, as given [here][] > > [here]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 > [disabled]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: revert removing @see tags where the URL was not the same as the @spec URL - Changes: - all: https://git.openjdk.org/jdk/pull/13248/files - new: https://git.openjdk.org/jdk/pull/13248/files/096a4188..7f64cc32 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13248&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13248&range=01-02 Stats: 12 lines in 4 files changed: 12 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13248.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13248/head:pull/13248 PR: https://git.openjdk.org/jdk/pull/13248
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1) [v2]
On Fri, 31 Mar 2023 10:45:39 GMT, Lance Andersen wrote: > > Hi Jon, > > This looks fine. I was wondering if we should do the same for java.util.zip > > and the PKWare Zip Spec or where java.sql references the JDBC Spec? > > Well, I must need coffee this morning as obviously JDBC is in the java.sql > module, not java.base So scratch that comment ;-) The other modules will be done in due course. - PR Comment: https://git.openjdk.org/jdk/pull/13248#issuecomment-1492417414
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1) [v2]
On Fri, 31 Mar 2023 17:14:01 GMT, Iris Clark wrote: >> Jonathan Gibbons has updated the pull request incrementally with one >> additional commit since the last revision: >> >> address review feedback > > src/java.base/share/classes/java/io/ObjectOutputStream.java line 165: > >> 163: * @see java.io.Serializable >> 164: * @see java.io.Externalizable >> 165: * @since 1.1 > > Just confirming... The changes to the java.io classes for the Serialization > Spec now all point to the index rather than particular chapters/sections. I'm > assuming that's intentional so that when the top-level Spec page appears, > there is a single entry for that specification. The `@spec` tag should point to the root, but we should not remove more specific references to within the spec. I will review places where `@see` has been removed/replaced. - PR Review Comment: https://git.openjdk.org/jdk/pull/13248#discussion_r1154774688
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1)
On Thu, 30 Mar 2023 17:24:11 GMT, Jonathan Gibbons wrote: > Please review a change to add `@spec` tags (and remove some equivalent `@see` > tags) to the main "core-libs" packages in `java.base` module. > > This is similar to, and a subset of, PR #11073. That PR was withdrawn, and > based on the ensuing discussion and suggestion, is now being handled with a > series of PRs for various separate parts of the system. Follow-up PRs will > be provided for the rest of `java.base`, for `java.desktop`, and for XML > APIs. The "LangTools" modules have already been updated. The "External > Specifications" page has been temporarily [disabled][] until this work is > complete. > > While the primary content of the change was automated, I've manually adjusted > the formatting, to break long lines. > > It is clear there is significant inconsistency in the ordering of block tags > in doc comment. We might want to (separately) consider normalizing the > order of the tags, perhaps according to the order defined for the tags in the > generated output, as given [here][] > > [here]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 > [disabled]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 > I skimmed through the changes and it looks quite good, much more workable > than PR 11073. > > Do you have a proposed ordering with other tags? I expected it would go with > @see but I see several where @SPEC is before @author, and @see after @author. > I know it doesn't really matter. The initial assumption was "after the @param/@return/@throws group". Overall, as I said in the description for this PR, the block tags are not very consistent about ordering. I was thinking we might want to recommend an overall ordering, but I'm worried it would be too intrusive a change to apply generally. In the description, I suggested an ordering based on the order in `Docs.gmk` which defines the order in which tags appear in the generated output. - PR Comment: https://git.openjdk.org/jdk/pull/13248#issuecomment-1490920050
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1) [v2]
> Please review a change to add `@spec` tags (and remove some equivalent `@see` > tags) to the main "core-libs" packages in `java.base` module. > > This is similar to, and a subset of, PR #11073. That PR was withdrawn, and > based on the ensuing discussion and suggestion, is now being handled with a > series of PRs for various separate parts of the system. Follow-up PRs will > be provided for the rest of `java.base`, for `java.desktop`, and for XML > APIs. The "LangTools" modules have already been updated. The "External > Specifications" page has been temporarily [disabled][] until this work is > complete. > > While the primary content of the change was automated, I've manually adjusted > the formatting, to break long lines. > > It is clear there is significant inconsistency in the ordering of block tags > in doc comment. We might want to (separately) consider normalizing the > order of the tags, perhaps according to the order defined for the tags in the > generated output, as given [here][] > > [here]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 > [disabled]: > https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: address review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/13248/files - new: https://git.openjdk.org/jdk/pull/13248/files/3e1102a9..096a4188 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13248&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13248&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13248.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13248/head:pull/13248 PR: https://git.openjdk.org/jdk/pull/13248
Re: RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1)
On Thu, 30 Mar 2023 19:42:33 GMT, Alan Bateman wrote: >> Please review a change to add `@spec` tags (and remove some equivalent >> `@see` tags) to the main "core-libs" packages in `java.base` module. >> >> This is similar to, and a subset of, PR #11073. That PR was withdrawn, and >> based on the ensuing discussion and suggestion, is now being handled with a >> series of PRs for various separate parts of the system. Follow-up PRs will >> be provided for the rest of `java.base`, for `java.desktop`, and for XML >> APIs. The "LangTools" modules have already been updated. The "External >> Specifications" page has been temporarily [disabled][] until this work is >> complete. >> >> While the primary content of the change was automated, I've manually >> adjusted the formatting, to break long lines. >> >> It is clear there is significant inconsistency in the ordering of block tags >> in doc comment. We might want to (separately) consider normalizing the >> order of the tags, perhaps according to the order defined for the tags in >> the generated output, as given [here][] >> >> [here]: >> https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 >> [disabled]: >> https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 > > src/java.base/share/classes/java/lang/Thread.java line 1960: > >> 1958: * thread. >> 1959: * >> 1960: * @spec jni/index.html Java Native Interface Specification > > The link to the JNI spec in this method is from implNote so I'm wondering if > the spec link is needed here. Right now, the tag is added for any declaration whose comment contains a reference to an external spec (i.e. with ``. When we enable the "External Specifications" page, it will contain a link back to this page as part of the cross-reference info, which seems useful. That being said, if you feel strongly the tag should not be added here, I can remove it. > src/java.base/share/classes/java/nio/file/Files.java line 1724: > >> 1722: * >> 1723: * @spec https://www.rfc-editor.org/info/rfc2045 >> 1724: * RFC 2045: RFC 2045: Multipurpose Internet Mail Extensions >> (MIME) Part One: Format of Internet Message Bodies > > Maybe this one can be put on two lines to avoid the wrapping when looking at > is side-by-side. Fixed. There was also a stutter-bug with a double "RFC 2045: RFC 2045:" which I have also fixed. - PR Review Comment: https://git.openjdk.org/jdk/pull/13248#discussion_r1153755656 PR Review Comment: https://git.openjdk.org/jdk/pull/13248#discussion_r1153756945
RFR: JDK-8305206: Add @spec tags in java.base/java.* (part 1)
Please review a change to add `@spec` tags (and remove some equivalent `@see` tags) to the main "core-libs" packages in `java.base` module. This is similar to, and a subset of, PR #11073. That PR was withdrawn, and based on the ensuing discussion and suggestion, is now being handled with a series of PRs for various separate parts of the system. Follow-up PRs will be provided for the rest of `java.base`, for `java.desktop`, and for XML APIs. The "LangTools" modules have already been updated. The "External Specifications" page has been temporarily [disabled][] until this work is complete. While the primary content of the change was automated, I've manually adjusted the formatting, to break long lines. It is clear there is significant inconsistency in the ordering of block tags in doc comment. We might want to (separately) consider normalizing the order of the tags, perhaps according to the order defined for the tags in the generated output, as given [here][] [here]: https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L68 [disabled]: https://github.com/openjdk/jdk/blob/83cf28f99639d80e62c4031c4c9752460de5f36c/make/Docs.gmk#L115 - Commit messages: - JDK-8305206: Add @spec tags in java.base/java.* (part 1) Changes: https://git.openjdk.org/jdk/pull/13248/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13248&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8305206 Stats: 281 lines in 60 files changed: 267 ins; 14 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13248.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13248/head:pull/13248 PR: https://git.openjdk.org/jdk/pull/13248
Re: RFR: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names [v2]
On Thu, 23 Mar 2023 22:34:24 GMT, Hannes Greule wrote: >> After merging master into https://github.com/openjdk/jdk/pull/9862, we >> encountered test failures (e.g., >> https://github.com/SirYwell/jdk/actions/runs/4500940829/jobs/7923018438#step:9:2541). >> The Classfile API tries to read from constant pool index 0 if a >> MethodParameters attribute has an entry without name. >> >> The fix is simply using `readUtf8EntryOrNull` instead of `readUtf8Entry`. >> The related code already correctly handles nullability. >> >> I didn't find an appropriate test class so I added a new one. Let me know if >> there's a better place or if the test can be improved somehow. >> >> As I don't have a JBS account, someone needs to create a bug report there >> for me. Thanks. > > Hannes Greule has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> test/jdk/jdk/classfile/BoundAttributeTest.java line 44: > 42: import static org.junit.jupiter.api.Assertions.assertTrue; > 43: > 44: /* Generally, the style for tests is to put the test description comment _before_ the imports. - PR Review Comment: https://git.openjdk.org/jdk/pull/13167#discussion_r1146955092
Re: RFR: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names [v2]
On Thu, 23 Mar 2023 22:34:24 GMT, Hannes Greule wrote: >> After merging master into https://github.com/openjdk/jdk/pull/9862, we >> encountered test failures (e.g., >> https://github.com/SirYwell/jdk/actions/runs/4500940829/jobs/7923018438#step:9:2541). >> The Classfile API tries to read from constant pool index 0 if a >> MethodParameters attribute has an entry without name. >> >> The fix is simply using `readUtf8EntryOrNull` instead of `readUtf8Entry`. >> The related code already correctly handles nullability. >> >> I didn't find an appropriate test class so I added a new one. Let me know if >> there's a better place or if the test can be improved somehow. >> >> As I don't have a JBS account, someone needs to create a bug report there >> for me. Thanks. > > Hannes Greule has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> test/jdk/jdk/classfile/BoundAttributeTest.java line 46: > 44: /* > 45: * @test > 46: * @issue 8304837 Should be `@bug`, not `@issue`. Did you run this test? Did it not complain at `@issue`? - PR Review Comment: https://git.openjdk.org/jdk/pull/13167#discussion_r1146952634
Re: RFR: 8301991: Convert l10n properties resource bundles to UTF-8 native
On Thu, 23 Feb 2023 09:04:23 GMT, Justin Lu wrote:
> This PR converts Unicode sequences to UTF-8 native in .properties file.
> (Excluding the Unicode space and tab sequence). The conversion was done using
> native2ascii.
>
> In addition, the build logic is adjusted to support reading in the
> .properties files as UTF-8 during the conversion from .properties file to
> .java ListResourceBundle file.
make/langtools/tools/compileproperties/CompileProperties.java line 252:
> 250: try {
> 251: writer = new BufferedWriter(
> 252: new OutputStreamWriter(new
> FileOutputStream(outputPath), StandardCharsets.ISO_8859_1));
Using ISO_8859_1 seems strange.
Since these are generated files, you could write them as UTF-8 and then
override the default javac option for ascii when compiling _just_ these files.
Or else just stay with ascii; no one should be looking at these files!
-
PR: https://git.openjdk.org/jdk/pull/12726
