Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v36]
On Fri, 16 Feb 2024 00:46:34 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with two > additional commits since the last revision: > > - Support for Table Of Contents in Markdown headings > - fix typo src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java line 286: > 284: lineKind = (ch == '\n' || ch == '\r') ? > LineKind.BLANK > 285: : (indent <= 3) ? peekLineKind() > 286: : lineKind != LineKind.OTHER ? > LineKind.INDENTED_CODE_BLOCK Doesn't this cause false positives for indented code blocks? In my understanding, indented lines in a list context and directly following a blockquote are not interpreted as code blocks, but as part of the list or blockquote. So my guess would be that `not OTHER` should be something like `BLANK or INDENTED_CODE_BLOCK or HEADER`, and that still leaves the problem of a [blank line in a list context](https://spec.commonmark.org/0.30/#example-108). - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1492065799
Re: RFR: 8325878: Require minimum Clang version 13
On Fri, 16 Feb 2024 04:46:24 GMT, Kim Barrett wrote: > > Unfortunately this will break my workflow on Linux - I use clang to build > > on Ubuntu 20.04, which is not that old, but it ships with clang 12. This is > > not a deal breaker, just annoying. > > That's unfortunate, but I think the [[noreturn]] issue is very important. > Until we have that fix, we can't rely on that attribute to silence certain > warnings. This requires us to continue to insert dead returns or apply other > workarounds. And forgetting to do so (because someone makes a change that > works fine with later versions or on other platforms) will lead to > build-breakage JBS issues/PRs just to continue to support older versions of > clang. We've already had several of those. Thank you for explaining the motivation. @forax also mentioned that our workaround produces false warnings in IDEs (just checked in CDT). I'm fine with it - I will find another solution for my Linux box. - PR Comment: https://git.openjdk.org/jdk/pull/17862#issuecomment-1947776707
Re: RFR: 8325878: Require minimum Clang version 13
On Thu, 15 Feb 2024 12:45:45 GMT, Thomas Stuefe wrote: > Unfortunately this will break my workflow on Linux - I use clang to build on > Ubuntu 20.04, which is not that old, but it ships with clang 12. This is not > a deal breaker, just annoying. That's unfortunate, but I think the [[noreturn]] issue is very important. Until we have that fix, we can't rely on that attribute to silence certain warnings. This requires us to continue to insert dead returns or apply other workarounds. And forgetting to do so (because someone makes a change that works fine with later versions or on other platforms) will lead to build-breakage JBS issues/PRs just to continue to support older versions of clang. We've already had several of those. - PR Comment: https://git.openjdk.org/jdk/pull/17862#issuecomment-1947750667
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:19:31 GMT, Magnus Ihse Bursie wrote: > Since jcheck only checks file in a commit, there is a possibility of us > getting files in the repository that would not be accepted by jcheck. This > can happen when extending the set of files checked by jcheck, or if jcheck > changes how it checks files (perhaps due to bug fixes). > > I have now run jcheck over the entire code base, and fixed a handful of > issues. With these changes, jcheck accept the entire code base. security properties looks ok. - Marked as reviewed by wetmore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17871#pullrequestreview-1884183930
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v36]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with two additional commits since the last revision: - Support for Table Of Contents in Markdown headings - fix typo - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/da8752c8..53afd804 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=35 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=34-35 Stats: 102 lines in 2 files changed: 99 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: Hotspot symbol visibility
Hi Magnus, For hotspot symbols that need to be exported, when statically linking the launcher executable using libjvm.a, we use lld's `-Wl,--export-dynamic-symbol-list=` option. Those exported symbols can be used outside the VM code, e.g. in agent. Our friend(s) in c++ compiler/toolchain added the support for --export-dynamic-symbol-list, https://reviews.llvm.org/D107317. -Wl,--dynamic-list with gcc provides similar support. Best, Jiangli On Wed, Feb 14, 2024 at 2:06 AM Magnus Ihse Bursie wrote: > > I am currently pursuing improved build functionality for static > libraries. One of the issues with static libraries are name collisions, > which led me back to an old discussion about which symbols are exported > from Hotspot, and how this is achieved. A long discussion is available > in JDK-8017234 [1], which was created in 2013 and has been on the back > burner ever since, coming back to life every now and then. > > There are basically two different problems here. They are both distinct > and interrelated, and we likely need to solve both in tandem. > > The first problem is how Hotspot should say which symbols (functions) > that should be exported from libjvm.so. The historical, and still used, > system is to have a mapfile. In the "new" (not so new anymore) build > system, this was simplified to a list of function names in > make/data/hotspot-symbols, from which a mapfile is built. > > This is in contrast with how all other libraries in the JDK are built, > and also with modern best practices. A better approach would be to > annotate the functions that should be exported in the source code. In > fact, such annotation already exists, even in Hotspot, as JNIEXPORT, but > this is currently not used in the compilation of Hotspot. > > The second problem is that in addition to these explicitly exported > functions, we also export basically all other functions as well in > Hotspot. (So currently we could basically just forgo this complicated > machinery and just export everything by default...) > > The reason for this seem somewhat unclear. The specifics are probably > forever lost in the mists of time past, but the essential point is that > these symbols are needed for SA to do it's job. > > So what we do is that we list all symbols in hotspot after compiling > (but before linking), and selects some (most, I think) of them using > regexp patterns, and add these to the mapfile. > > As long as we're doing this, we cannot stop using a mapfile, and we're > stuck from progressing on point 1. > > My takeaway from the discussions is that we are most likely exporting a > way too broad set of symbols to SA. It seems likely that this set can be > drastically cut down. Actually getting an understanding of exactly which > symbols SA need seem like a very good first step. > > So, is this a journey anyone would be interested in embarkering on? I > will help as much as I can from a build PoV, but I have no clue > whatsoever about what SA needs. > > /Magnus > > [1] https://bugs.openjdk.org/browse/JDK-8017234 >
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:19:31 GMT, Magnus Ihse Bursie wrote: > Since jcheck only checks file in a commit, there is a possibility of us > getting files in the repository that would not be accepted by jcheck. This > can happen when extending the set of files checked by jcheck, or if jcheck > changes how it checks files (perhaps due to bug fixes). > > I have now run jcheck over the entire code base, and fixed a handful of > issues. With these changes, jcheck accept the entire code base. desktop changes look fine to me. - Marked as reviewed by prr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17871#pullrequestreview-1884009342
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v35]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with three additional commits since the last revision: - fix handling of `@' in a Markdown comment - improve comments for some LineKind members - update LineKind members and test - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/393d3a9c..da8752c8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=34 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=33-34 Stats: 55 lines in 3 files changed: 49 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: Questions about the Hermetic Java project
On Wed, Feb 14, 2024 at 5:07 PM Jiangli Zhou wrote: > > Hi Magnus, > > Thanks for looking into this from the build perspective. > > On Wed, Feb 14, 2024 at 1:00 AM Magnus Ihse Bursie > wrote: > > > > First some background for build-dev: I have spent some time looking at > > the build implications of the Hermetic Java effort, which is part of > > Project Leyden. A high-level overview is available here: > > https://cr.openjdk.org/~jiangli/hermetic_java.pdf and the current source > > code is here: https://github.com/openjdk/leyden/tree/hermetic-java-runtime. > > Some additional hermetic Java related references that are also useful: > > - https://bugs.openjdk.org/browse/JDK-8303796 is an umbrella bug that > links to the issues for resolving static linking issues so far > - https://github.com/openjdk/jdk21/pull/26 is the enhancement for > building the complete set of static libraries in JDK/VM, particularly > including libjvm.a > > > > > Hermetic Java faces several challenges, but the part that is relevant > > for the build system is the ability to create static libraries. We've > > had this functionality (in three different ways...) for some time, but > > it is rather badly implemented. > > > > As a result of my investigations, I have a bunch of questions. :-) I > > have gotten some answers in private discussion, but for the sake of > > transparency I will repeat them here, to foster an open dialogue. > > > > 1. Am I correct in understanding that the ultimate goal of this exercise > > is to be able to have jmods which include static libraries (*.a) of the > > native code which the module uses, and that the user can then run a > > special jlink command to have this linked into a single executable > > binary (which also bundles the *.class files and any additional > > resources needed)? > > > > 2. If so, is the idea to create special kinds of static jmods, like > > java.base-static.jmod, that contains *.a files instead of lib*.so files? > > Or is the idea that the normal jmod should contain both? > > > > 3. Linking .o and .a files into an executable is a formidable task. Is > > the intention to have jlink call a system-provided ld, or to bundle ld > > with jlink, or to reimplement this functionality in Java? > > I have a similar view as Alan responded in your other email thread. > Things are still in the early stage for the general solution. > > In the https://github.com/openjdk/leyden/tree/hermetic-java-runtime > branch, when configuring JDK with --with-static-java=yes, the JDK > binary contains the following extra artifacts: > > - static-libs/*.a: The complete set of JDK/VM static libraries > - jdk/bin/javastatic: A demo Java launcher fully statically linked > with the selected JDK .a libraries (e.g. it currently statically link > with the headless) and libjvm.a. It's the standard Java launcher > without additional work for hermetic Java. > > In our prototype for hermetic Java, we build the hermetic executable > image (a single image) from the following input (see description on > singlejar packaging tool in > https://cr.openjdk.org/~jiangli/hermetic_java.pdf): > > - A customized launcher (with additional work for hermetic) executable > fully statically linked with JDK/VM static libraries (.a files), > application natives and dependencies (e.g. in .a static libraries) > - JDK lib/modules, JDK resource files > - Application classes and resource files > > Including a JDK library .a into the corresponding .jmod would require > extracting the .a for linking with the executable. In some systems > that may cause memory overhead due to the extracted copy of the .a > files. I think we should consider the memory overhead issue. > > One possibility (as Alan described in his response) is for jlink to > invoke the ld on the build system. jlink could pass the needed JDK > static libraries and libjvm.a (provided as part of the JDK binary) to > ld based on the modules required for the application. > I gave a bit more thoughts on this one. For jlink to trigger ld, it would need to know the complete linker options and inputs. Those include options and inputs related to the application part as well. In some usages, it might be easier to handle native linking separately and pass the linker output, the executable to jlink directly. Maybe we could consider supporting different modes for various usages requirements, from static libraries and native linking point of view: Mode #1 Support .jmod packaged natives static libraries, for both JDK/VM .a and application natives and dependencies. If the inputs to jlink include .jmods, jlink can extract the .a libraries and pass the information to ld to link the executable. Mode #2 Support separate .a as jlink input. Jlink could pass the path information to the .a libraries and other linker options to ld to create the executable. For both mode #1 and #2, jlink would then use the linker output executable to create the final hermetic image. Mode #3 Support a fully linked executable as a
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 19:39:07 GMT, Pavel Rappo wrote:
>> whitespace is handled separately, on line 280 (`readIndent`) and285 (`:
>> (indent <= 3) ? peekLineKind()`)
>
> Correct, but I believe the ordered list marker should be like this:
>
> ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
> ^
>
> Note extra whitespace character. I think we should really add this to our
> tests, since lists are foundational Markdown structures, probably right after
> italics and bold.
Then we should add `[ \t]` to both constants, right:
BULLETED_LIST_ITEM(Pattern.compile("[-+*][ \t].*"))
ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)][ \t].*"))
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491564839
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v31]
On Thu, 15 Feb 2024 19:36:36 GMT, Jonathan Gibbons wrote:
>> 1. Since forever, and still true, the way to specify a doclet is by its
>> name, and the tool will create the instance for you. This goes back to the
>> original old days before any API, when the only entry point was the command
>> line.
>> This implies that (a) the doclet has a no-args constructor and (b) that all
>> doclet config is done through command line options. A better solution would
>> be to add new functionality to the various javadoc tool API (some or all of
>> `Main`/`Start`/`DocumentationTool`) so that a client can initialize an
>> instance of a doclet and pass that instance into the API.
>>
>> 2. If I understand the question correctly, the code is invoked by using the
>> command-line option `-XDaccessInternalAPI` which is a preexisting hidden
>> feature already supported by `javac`. It is used in `TestTransformer.java`
>> on line 161.
>>
>> javadoc("-d", base.resolve("api").toString(),
>> "-Xdoclint:none",
>> "-XDaccessInternalAPI", // required to access JavacTrees
>> "-doclet", "TestTransformer$MyDoclet",
>> "-docletpath", System.getProperty("test.classes"),
>> "-sourcepath", src.toString(),
>> "p");
>
> I confirm that `TestTransformer.java` fails as expected with an
> `IllegalAccessError` if the option is not used.
>
> java.lang.IllegalAccessError: class TestTransformer$MyDoclet (in unnamed
> module @0x355de863) cannot access class com.sun.tools.javac.api.JavacTrees
> (in module jdk.compiler) because module jdk.compiler does not export
> com.sun.tools.javac.api to unnamed module @0x355de863
> at TestTransformer$MyDoclet.run(TestTransformer.java:139)
> at
> jdk.javadoc/jdk.javadoc.internal.tool.Start.parseAndExecute(Start.java:589)
Generally, the error occurs because the `MyDoclet` class is run in a different
class loader than that used for the test. The class loader for the test
already has the necessary access permissions given, from the lines in
`@modules` in the `jtreg` test description. Ideally, we could call `new
MyDoclet()` in the main test code, and pas the instance in to the `javadoc`
call and from there to the javadoc `Start` method.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491547571
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 19:39:07 GMT, Pavel Rappo wrote:
>> whitespace is handled separately, on line 280 (`readIndent`) and285 (`:
>> (indent <= 3) ? peekLineKind()`)
>
> Correct, but I believe the ordered list marker should be like this:
>
> ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
> ^
>
> Note extra whitespace character. I think we should really add this to our
> tests, since lists are foundational Markdown structures, probably right after
> italics and bold.
My recollection is that the space is required. I will double check.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491552312
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 19:20:23 GMT, Jonathan Gibbons wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1401:
>>
>>> 1399: */
>>> 1400: enum LineKind {
>>> 1401: BLANK(Pattern.compile("[ \t]*")),
>>
>> `BLANK` is a pseudo kind, because it is set manually, but never returned
>> from `peekLine()`. I wonder if we can change it somehow.
>
> Even if it is set manually, it is still appropriate to have it as a member in
> the `LineKind` enum class.
Not that it invalidates your reply; clarification: I meant `peekLineKind()`,
not `peekLine()`.
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1433:
>>
>>> 1431: * @see >> href="https://spec.commonmark.org/0.30/#list-items";>List items
>>> 1432: */
>>> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
>>
>> This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM`
>> constants. I know that we don't need to be very precise, but perhaps in this
>> case we should. While the CommonMark spec is a vague on that, from my
>> experiments with [dingus](https://spec.commonmark.org/dingus/), it appears
>> that a list marker can be preceded and followed by some number of whitespace
>> characters.
>
> whitespace is handled separately, on line 280 (`readIndent`) and285 (`:
> (indent <= 3) ? peekLineKind()`)
Correct, but I believe the ordered list marker should be like this:
ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
^
Note extra whitespace character. I think we should really add this to our
tests, since lists are foundational Markdown structures, probably right after
italics and bold.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491550784
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491545609
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v31]
On Thu, 15 Feb 2024 19:15:25 GMT, Jonathan Gibbons wrote:
>> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Start.java line 571:
>>
>>> 569: // of a doclet to be specified instead of the name of the
>>> 570: // doclet class and optional doclet path.
>>> 571: // See https://bugs.openjdk.org/browse/JDK-8263219
>>
>> It's hard to understand this:
>>
>>> to permit an instance of an appropriately configured instance of a doclet
>>
>> Also: how is that piece of code used? When I commented it out, no
>> test/langtools:langtools_javadoc tests failed.
>
> 1. Since forever, and still true, the way to specify a doclet is by its
> name, and the tool will create the instance for you. This goes back to the
> original old days before any API, when the only entry point was the command
> line.
> This implies that (a) the doclet has a no-args constructor and (b) that all
> doclet config is done through command line options. A better solution would
> be to add new functionality to the various javadoc tool API (some or all of
> `Main`/`Start`/`DocumentationTool`) so that a client can initialize an
> instance of a doclet and pass that instance into the API.
>
> 2. If I understand the question correctly, the code is invoked by using the
> command-line option `-XDaccessInternalAPI` which is a preexisting hidden
> feature already supported by `javac`. It is used in `TestTransformer.java`
> on line 161.
>
> javadoc("-d", base.resolve("api").toString(),
> "-Xdoclint:none",
> "-XDaccessInternalAPI", // required to access JavacTrees
> "-doclet", "TestTransformer$MyDoclet",
> "-docletpath", System.getProperty("test.classes"),
> "-sourcepath", src.toString(),
> "p");
I confirm that `TestTransformer.java` fails as expected with an
`IllegalAccessError` if the option is not used.
java.lang.IllegalAccessError: class TestTransformer$MyDoclet (in unnamed module
@0x355de863) cannot access class com.sun.tools.javac.api.JavacTrees (in module
jdk.compiler) because module jdk.compiler does not export
com.sun.tools.javac.api to unnamed module @0x355de863
at TestTransformer$MyDoclet.run(TestTransformer.java:139)
at
jdk.javadoc/jdk.javadoc.internal.tool.Start.parseAndExecute(Start.java:589)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491538120
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v34]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with two additional commits since the last revision: - fix return tag name - remove debug print - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/f6d08db9..393d3a9c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=33 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=32-33 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 19:27:12 GMT, Jonathan Gibbons wrote: >> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java >> line 422: >> >>> 420: defaultContentCharacter(); >>> 421: } >>> 422: } >> >> Is it to pass `` through to Markdown, to allow it to deal with Markdown >> escapes? > > It is more about supporting the sequence `` ` `` so that the backtick is > treated as a literal character and not the beginning of a code span. This > is the "backstop" preferred way to ensure that a single backtick is treated > literally, without relying on detected that it is unbalanced when the end of > the current block is reached. The alternative workaround would be a single > backtick enclosed in multiple backticks, such as this ``` `` ` `` ```. (I > leave you to figure out what I actually typed there!!!) You might also need to use `` ` `` when there are two literal backticks in a sentence. After the first backtick (`), another backtick (`) can be used to delimit a code span. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491528153
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 17:17:46 GMT, Pavel Rappo wrote: >> Jonathan Gibbons has updated the pull request with a new target base due to >> a merge or a rebase. The pull request now contains 44 commits: >> >> - fill in `visitRawText` in `CommentHelper.getTags` visitor >> - fixes for the "New API" page >> - change "standard" to "traditional" when referring to a comment >> - Merge remote-tracking branch 'upstream/master' into >> 8298405.doclet-markdown-v3 >> - Merge remote-tracking branch 'upstream/master' into >> 8298405.doclet-markdown-v3 >> - improve support for DocCommentParser.LineKind >> - Merge remote-tracking branch 'upstream/master' into >> 8298405.doclet-markdown-v3 # 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. >> - update copyright year on test >> - refactor recent new test case in TestMarkdown.java >> - address review feedback >> - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1 > > src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java > line 422: > >> 420: defaultContentCharacter(); >> 421: } >> 422: } > > Is it to pass `` through to Markdown, to allow it to deal with Markdown > escapes? It is more about supporting the sequence `` ` `` so that the backtick is treated as a literal character and not the beginning of a code span. This is the "backstop" preferred way to ensure that a single backtick is treated literally, without relying on detected that it is unbalanced when the end of the current block is reached. The alternative workaround would be a single backtick enclosed in multiple backticks, such as this ``` `` ` `` ```. (I leave you to figure out what I actually typed there!!!) - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491519707
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 17:03:09 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains 44 commits:
>>
>> - fill in `visitRawText` in `CommentHelper.getTags` visitor
>> - fixes for the "New API" page
>> - change "standard" to "traditional" when referring to a comment
>> - Merge remote-tracking branch 'upstream/master' into
>> 8298405.doclet-markdown-v3
>> - Merge remote-tracking branch 'upstream/master' into
>> 8298405.doclet-markdown-v3
>> - improve support for DocCommentParser.LineKind
>> - Merge remote-tracking branch 'upstream/master' into
>> 8298405.doclet-markdown-v3 # 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.
>> - update copyright year on test
>> - refactor recent new test case in TestMarkdown.java
>> - address review feedback
>> - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1401:
>
>> 1399: */
>> 1400: enum LineKind {
>> 1401: BLANK(Pattern.compile("[ \t]*")),
>
> `BLANK` is a pseudo kind, because it is set manually, but never returned from
> `peekLine()`. I wonder if we can change it somehow.
Even if it is set manually, it is still appropriate to have it as a member in
the `LineKind` enum class.
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1433:
>
>> 1431: * @see > href="https://spec.commonmark.org/0.30/#list-items";>List items
>> 1432: */
>> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
>
> This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM` constants.
> I know that we don't need to be very precise, but perhaps in this case we
> should. While the CommonMark spec is a vague on that, from my experiments
> with [dingus](https://spec.commonmark.org/dingus/), it appears that a list
> marker can be preceded and followed by some number of whitespace characters.
whitespace is handled separately, on line 280 (`readIndent`) and285 (`: (indent
<= 3) ? peekLineKind()`)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491508303
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491512611
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v31]
On Tue, 13 Feb 2024 11:15:55 GMT, Pavel Rappo wrote:
>> Jonathan Gibbons has updated the pull request with a new target base due to
>> a merge or a rebase. The pull request now contains 40 commits:
>>
>> - Merge remote-tracking branch 'upstream/master' into
>> 8298405.doclet-markdown-v3
>> - improve support for DocCommentParser.LineKind
>> - Merge remote-tracking branch 'upstream/master' into
>> 8298405.doclet-markdown-v3 # 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.
>> - update copyright year on test
>> - refactor recent new test case in TestMarkdown.java
>> - address review feedback
>> - address review feedback
>> - added test case in TestMarkdown.java for handling of `@deprecated` tag
>> - amend comment in test
>> - improve comments on negative test case
>> - ... and 30 more: https://git.openjdk.org/jdk/compare/2ed889b7...1c64a6e0
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclint/Checker.java line
> 1021:
>
>> 1019: .findFirst();
>> 1020: if (first.isEmpty() || first.get() != tree) {
>> 1021: dct.getFirstSentence().forEach(t ->
>> System.err.println(t.getKind() + ": >>|" + t + "|<<"));
>
> Is it leftover debug output?
Oops, yes. Thanks.
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/tool/Start.java line 571:
>
>> 569: // of a doclet to be specified instead of the name of the
>> 570: // doclet class and optional doclet path.
>> 571: // See https://bugs.openjdk.org/browse/JDK-8263219
>
> It's hard to understand this:
>
>> to permit an instance of an appropriately configured instance of a doclet
>
> Also: how is that piece of code used? When I commented it out, no
> test/langtools:langtools_javadoc tests failed.
1. Since forever, and still true, the way to specify a doclet is by its name,
and the tool will create the instance for you. This goes back to the original
old days before any API, when the only entry point was the command line.
This implies that (a) the doclet has a no-args constructor and (b) that all
doclet config is done through command line options. A better solution would be
to add new functionality to the various javadoc tool API (some or all of
`Main`/`Start`/`DocumentationTool`) so that a client can initialize an instance
of a doclet and pass that instance into the API.
2. If I understand the question correctly, the code is invoked by using the
command-line option `-XDaccessInternalAPI` which is a preexisting hidden
feature already supported by `javac`. It is used in `TestTransformer.java` on
line 161.
javadoc("-d", base.resolve("api").toString(),
"-Xdoclint:none",
"-XDaccessInternalAPI", // required to access JavacTrees
"-doclet", "TestTransformer$MyDoclet",
"-docletpath", System.getProperty("test.classes"),
"-sourcepath", src.toString(),
"p");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491504223
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491502389
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v33]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: fix compilation and whitespace issues - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/2801c2e1..f6d08db9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=32 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=31-32 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
Re: RFR: 8325972: Add -x to bash for building with LOG=debug
On Thu, 15 Feb 2024 15:07:46 GMT, Magnus Ihse Bursie wrote: > I don't understand why I have never thought of this before. If we add `-x` to > the set of bash arguments when running with LOG=debug, we get output of *all* > shell commands that make is running, even those for $(shell). > > This makes it s much easier to understand what is actually happening in > the makefile! (To the point where we could actually consider moving other > stuff away from the debug level.) Should we also disable the built in make command printing option for `debug` to avoid duplicate output? - PR Review: https://git.openjdk.org/jdk/pull/17875#pullrequestreview-1883482256
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 17:28:52 GMT, Andy Goryachev wrote: >> Please do not replace those tabs with spaces as they are intentional for >> testing the runtime to safely ignore them. I suggest replacing them with >> Unicode escapes (`\u000b`) > > `\u000b` is VT (vertical tab) > `\u0009` or `\t` perhaps? Right. `\t` is better to avoid such a mistake. - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491403426
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 17:09:17 GMT, Naoto Sato wrote: >> All the java/util/Currency tests pass. I also searched the code for "ZZ" but >> could not find any references in the test. > > Please do not replace those tabs with spaces as they are intentional for > testing the runtime to safely ignore them. I suggest replacing them with > Unicode escapes (`\u000b`) `\u000b` is VT (vertical tab) `\u0009` or `\t` perhaps? - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491375928
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]
On Thu, 15 Feb 2024 00:30:25 GMT, Jonathan Gibbons wrote:
>> Please review a patch to add support for Markdown syntax in documentation
>> comments, as described in the associated JEP.
>>
>> Notable features:
>>
>> * support for `///` documentation comments in `JavaTokenizer`
>> * new module `jdk.internal.md` -- a private copy of the `commonmark-java`
>> library
>> * updates to `DocCommentParser` to treat `///` comments as Markdown
>> * updates to the standard doclet to render Markdown comments in HTML
>
> Jonathan Gibbons has updated the pull request with a new target base due to a
> merge or a rebase. The pull request now contains 44 commits:
>
> - fill in `visitRawText` in `CommentHelper.getTags` visitor
> - fixes for the "New API" page
> - change "standard" to "traditional" when referring to a comment
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3
> - improve support for DocCommentParser.LineKind
> - Merge remote-tracking branch 'upstream/master' into
> 8298405.doclet-markdown-v3 # 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.
> - update copyright year on test
> - refactor recent new test case in TestMarkdown.java
> - address review feedback
> - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1
I'm again looking `LineKind`. This time because of 9eaf84e5dd6.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 422:
> 420: defaultContentCharacter();
> 421: }
> 422: }
Is it to pass `` through to Markdown, to allow it to deal with Markdown escapes?
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1401:
> 1399: */
> 1400: enum LineKind {
> 1401: BLANK(Pattern.compile("[ \t]*")),
`BLANK` is a pseudo kind, because it is set manually, but never returned from
`peekLine()`. I wonder if we can change it somehow.
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1433:
> 1431: * @see href="https://spec.commonmark.org/0.30/#list-items";>List items
> 1432: */
> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM` constants. I
know that we don't need to be very precise, but perhaps in this case we should.
While the CommonMark spec is a vague on that, from my experiments with
[dingus](https://spec.commonmark.org/dingus/), it appears that a list marker
can be preceded and followed by some number of whitespace characters.
-
PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1883374712
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491362821
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491344667
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491354450
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Mon, 5 Feb 2024 09:44:02 GMT, Magnus Ihse Bursie wrote: > > Sure, you can always install a newer GCC than the system one, but it's > > another thing that makes it harder for people to build OpenJDK. Having said > > that, C++17 is nice to have. > > @theRealAph I am still just hearing hand-waving "perhaps someone somewhere > will have a somewhat harder time building the JDK". Yes, perhaps that is so. > But that is very uncertain, and I have still not heard a single concrete > example of where this would apply. In contrast, going to gcc 10 will clearly > bring a benefit to all other platforms, since it allows us to synchronize the > code base at C++17. > > In light of this, I think we need to hear some really compelling evidence of > problems that would ensue if we raise the minimum to gcc 10. If nobody can > produce such evidence, then to me it is a sign that this fear is not > well-grounded, and we should proceed with this PR. @magicus You put the onus of proving that problems could ensue strictly to the objectors. That is a bit one-sided. I do not see much effort - any, really - put into detailing the motivation for this move, neither in the PR description nor in the JBS issue text. I just read through the whole PR discussion and really the only helpful comment I found was from Kim ( https://github.com/openjdk/jdk/pull/14988#issuecomment-1858136247 ). I think important decisions like enforcing C++17 would benefit from a more careful preparation. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1946628523
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 15:48:38 GMT, Magnus Ihse Bursie wrote: >> This looks weird indeed. Luckily it's just test code. Did you run the test >> after this change? > > All the java/util/Currency tests pass. I also searched the code for "ZZ" but > could not find any references in the test. Please do not replace those tabs with spaces as they are intentional for testing the runtime to safely ignore them. I suggest replacing them with Unicode escapes (`\u000b`) - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491352359
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Thu, 15 Feb 2024 15:54:56 GMT, Magnus Ihse Bursie wrote: > > I would like it if toolchain version bumps were discussed somewhere else > > first, not in a PR. (And apologies if it was and I missed that discussion). > > Yes, it definitely was. I posted a separate [mail to > build-dev](https://mail.openjdk.org/pipermail/build-dev/2024-January/042802.html) > with subject "Raising the minimum version of gcc, clang and xlc". I don't > think we can make it more clear than that. Okay, thank you for that clarification. I clearly missed it then. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1946396577
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Thu, 15 Feb 2024 13:00:58 GMT, Thomas Stuefe wrote: > I would like it if toolchain version bumps were discussed somewhere else > first, not in a PR. (And apologies if it was and I missed that discussion). Yes, it definitely was. I posted a separate [mail to build-dev](https://mail.openjdk.org/pipermail/build-dev/2024-January/042802.html) with subject "Raising the minimum version of gcc, clang and xlc". I don't think we can make it more clear than that. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1946388775
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. Sounds like it really should be `globalDefinitions_aix.hpp` then... - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1946379438
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 14:01:46 GMT, Erik Joelsson wrote: >> test/jdk/java/util/Currency/currency.properties line 18: >> >>> 16: SB=EUR,111,2, 2099-01-01T00:00:00 >>> 17: US=euR,978,2,2001-01-01T00:00:00 >>> 18: ZZ = ZZZ , 999 , 3 >> >> This looks weird, but so did the original code. I assume this is to clearly >> indicate this as a end of list marker. > > This looks weird indeed. Luckily it's just test code. Did you run the test > after this change? All the java/util/Currency tests pass. I also searched the code for "ZZ" but could not find any references in the test. - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491227492
RFR: 8325972: Add -x to bash for building with LOG=debug
I don't understand why I have never thought of this before. If we add `-x` to the set of bash arguments when running with LOG=debug, we get output of *all* shell commands that make is running, even those for $(shell). This makes it s much easier to understand what is actually happening in the makefile! (To the point where we could actually consider moving other stuff away from the debug level.) - Commit messages: - 8325972: Add -x to bash for building with LOG=debug Changes: https://git.openjdk.org/jdk/pull/17875/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17875&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325972 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17875.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17875/head:pull/17875 PR: https://git.openjdk.org/jdk/pull/17875
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:26:11 GMT, Magnus Ihse Bursie wrote: >> Since jcheck only checks file in a commit, there is a possibility of us >> getting files in the repository that would not be accepted by jcheck. This >> can happen when extending the set of files checked by jcheck, or if jcheck >> changes how it checks files (perhaps due to bug fixes). >> >> I have now run jcheck over the entire code base, and fixed a handful of >> issues. With these changes, jcheck accept the entire code base. > > test/jdk/java/util/Currency/currency.properties line 18: > >> 16: SB=EUR,111,2, 2099-01-01T00:00:00 >> 17: US=euR,978,2,2001-01-01T00:00:00 >> 18: ZZ = ZZZ , 999 , 3 > > This looks weird, but so did the original code. I assume this is to clearly > indicate this as a end of list marker. This looks weird indeed. Luckily it's just test code. Did you run the test after this change? - PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1491056108
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 12:49:26 GMT, Julian Waters wrote: > > > I see. I believe I wrote that piece of code, but I'd clearly forgotten > > > that. 😕 Thanks! :) > > > > > > No, this was added by me, because this was the root point to still resolve > > to globalDefinitions_xlc.hpp even with toolchain clang > > Shame that we can't fully swap to clang in some areas for AIX, but oh well Although the compiler is now clang, the headers are still the old IBM ones and globalDefinitions_xlc.hpp is not consumable by other clang implementations. So if we change this we still have to differentiate between AIX clang and other clangs. - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1946086792
Re: RFR: 8314488: Compile the JDK as C++17 [v6]
On Thu, 11 Jan 2024 13:23:45 GMT, Julian Waters wrote: >> Compile the JDK as C++17, enabling the use of all C++17 language features > > Julian Waters has updated the pull request incrementally with one additional > commit since the last revision: > > Require clang 13 in toolchain.m4 Just on a general note: I would like it if toolchain version bumps were discussed somewhere else first, not in a PR. (And apologies if it was and I missed that discussion). Because PRs are usually followed only by active developers, but toolchain versions affect a broader part of the community. As we have seen in this PR, there are conflicting interests. We have things like CSRs and JEPs. Both are not ideal - a JEP is way too massive, and a CSR is about the compatibility of the product, not about build-time. Still, maybe something like a CSR would make sense here. I understand that with toolchain support, there will always be opposing parties, and I can see arguments on both sides. We don't want to end up like FreeBSD - stuck in time because of opinion stalemates. I just keep thinking that a PR is maybe not the perfect forum for this. - PR Comment: https://git.openjdk.org/jdk/pull/14988#issuecomment-1946053820
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 12:36:25 GMT, Joachim Kern wrote: > > I see. I believe I wrote that piece of code, but I'd clearly forgotten > > that. 😕 Thanks! :) > > No, this was added by me, because this was the root point to still resolve to > globalDefinitions_xlc.hpp even with toolchain clang Shame that we can't fully swap to clang in some areas for AIX, but oh well - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1946036007
Re: RFR: 8325878: Require minimum Clang version 13
On Thu, 15 Feb 2024 05:19:45 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of Clang > to be used for building OpenJDK from 3.5 to 13. > > This permits enabling C++17 (JDK-8314488), though Clang 5 might suffice for > that. A minimum of Clang 13 also obtains a critical bug fix for the > [[noreturn]] > attribute (see JDK-8303805). > > Testing: mach5 tier1, which includes building with a recent version of > Xcode/clang. Unfortunately this will break my workflow on Linux - I use clang to build on Ubuntu 20.04, which is not that old, but it ships with clang 12. This is not a deal breaker, just annoying. - PR Comment: https://git.openjdk.org/jdk/pull/17862#issuecomment-1946030283
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 12:29:50 GMT, Magnus Ihse Bursie wrote: > I see. I believe I wrote that piece of code, but I'd clearly forgotten that. > 😕 Thanks! :) No, this was added by me, because this was the root point to still resolve to globalDefinitions_xlc.hpp even with toolchain clang - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1946015507
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. I see. I believe I wrote that piece of code, but I'd clearly forgotten that. 😕 Thanks! :) - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1946004231
Re: RFR: 8325950: Make sure all files in the JDK pass jcheck
On Thu, 15 Feb 2024 12:19:31 GMT, Magnus Ihse Bursie wrote: > Since jcheck only checks file in a commit, there is a possibility of us > getting files in the repository that would not be accepted by jcheck. This > can happen when extending the set of files checked by jcheck, or if jcheck > changes how it checks files (perhaps due to bug fixes). > > I have now run jcheck over the entire code base, and fixed a handful of > issues. With these changes, jcheck accept the entire code base. Some general remarks: The problems in .m4 files where not properly detected and fixed when I added .m4 to the list of checked files. The properties files were recently checked by me, so these suprrised me. It turned out I had misunderstood the jcheck criteria: I thought only leading tabs were disallowed, but it is actually tabs anywhere in the file that are banned. In general, I have replaced tab characters with spaces aligning to tab stops at 8 characters distance. I'll leave some remarks for individual files. src/java.naming/share/classes/com/sun/jndi/ldap/jndiprovider.properties line 10: > 8: > java.naming.factory.object=com.sun.jndi.ldap.obj.AttrsToCorba:com.sun.jndi.ldap.obj.MarshalledToObject > 9: > 10: # RemoteToAttrs: Turn RMI/JRMP and RMI/IIOP object into > javaMarshalledObject or I adjusted this tab stop (with one space) so it aligned properly with the line above; I think that was the intention. src/jdk.jdeps/share/classes/com/sun/tools/jdeps/resources/jdkinternals.properties line 41: > 39: jdk.internal.ref.Cleaner=Use java.lang.ref.PhantomReference @since 1.2 or > java.lang.ref.Cleaner @since 9 > 40: sun.awt.CausedFocusEvent=Use java.awt.event.FocusEvent::getCause @since 9 > 41: sun.font.FontUtilities=See java.awt.Font.textRequiresLayout @since 9 This tab just looked out of place, so I replaced it with a space. (Rhyming not intentional...) test/hotspot/jtreg/containers/docker/JfrReporter.java line 52: > 50: } > 51: } > 52: } I'm not sure how a Java file ever got into the code base with trailing spaces, but a guess is that there have been a bug in jcheck that did not properly check for trailing whitespace at the end of the file, or something like that. test/jdk/java/util/Currency/currency.properties line 18: > 16: SB=EUR,111,2, 2099-01-01T00:00:00 > 17: US=euR,978,2,2001-01-01T00:00:00 > 18: ZZ= ZZZ , 999 , 3 This looks weird, but so did the original code. I assume this is to clearly indicate this as a end of list marker. - PR Comment: https://git.openjdk.org/jdk/pull/17871#issuecomment-1945992837 PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1490931378 PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1490931979 PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1490933432 PR Review Comment: https://git.openjdk.org/jdk/pull/17871#discussion_r1490934063
RFR: 8325950: Make sure all files in the JDK pass jcheck
Since jcheck only checks file in a commit, there is a possibility of us getting files in the repository that would not be accepted by jcheck. This can happen when extending the set of files checked by jcheck, or if jcheck changes how it checks files (perhaps due to bug fixes). I have now run jcheck over the entire code base, and fixed a handful of issues. With these changes, jcheck accept the entire code base. - Commit messages: - 8325950: Make sure all files in the JDK pass jcheck Changes: https://git.openjdk.org/jdk/pull/17871/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17871&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325950 Stats: 113 lines in 38 files changed: 0 ins; 10 del; 103 mod Patch: https://git.openjdk.org/jdk/pull/17871.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17871/head:pull/17871 PR: https://git.openjdk.org/jdk/pull/17871
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 11:10:32 GMT, Joachim Kern wrote: >> What does this mean? That you are not using xlc at all? Or is it clang but >> still with an xlc frontend, so all xlc flags etc need to stay? > >> What does this mean? That you are not using xlc at all? Or is it clang but >> still with an xlc frontend, so all xlc flags etc need to stay? > > > The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the > `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and higher. > For the 17 Compiler the frontend is `clang`-ish and we are using the `clang` > flags instead of the `xlc` flags. > `toolchain.m4` decides on the basis of the found compiler which toolchain to > use as default. > > # On AIX the default toolchain depends on the installed (found) compiler > # xlclang++ -> xlc toolchain > # ibm-clang++_r -> clang toolchain > # The compiler is searched on the PATH and TOOLCHAIN_PATH > # xlclang++ has precedence over ibm-clang++_r if both are installed > > So, if we set the minimum compiler level for AIX to 17, we can remove the xlc > toolchain at all. > We cannot remove every reference to xlc, because at least some headers we > still use the xlc version (globalDefinitions_xlc.hpp) > @JoKern65 Thanks for the explanation! It would be nice to clear out the xlc > stuff. Let's open a separate issue for that once this is integrated. > > I also believe that the selection of `globalDefinitions_xlc.hpp` is done by > #ifdefs in Hotspot source code, and has no relation to the build systems > concept of toolchains. There is one spot in the build system where clang is forcefully labelled as xlc, in the place where the HotSpot toolchain type is set https://github.com/openjdk/jdk/blob/a0e5e16afbd19f6396f0af2cba954225a357eca8/make/autoconf/toolchain.m4#L1015 - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1945992143
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 11:10:32 GMT, Joachim Kern wrote: >> What does this mean? That you are not using xlc at all? Or is it clang but >> still with an xlc frontend, so all xlc flags etc need to stay? > >> What does this mean? That you are not using xlc at all? Or is it clang but >> still with an xlc frontend, so all xlc flags etc need to stay? > > > The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the > `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and higher. > For the 17 Compiler the frontend is `clang`-ish and we are using the `clang` > flags instead of the `xlc` flags. > `toolchain.m4` decides on the basis of the found compiler which toolchain to > use as default. > > # On AIX the default toolchain depends on the installed (found) compiler > # xlclang++ -> xlc toolchain > # ibm-clang++_r -> clang toolchain > # The compiler is searched on the PATH and TOOLCHAIN_PATH > # xlclang++ has precedence over ibm-clang++_r if both are installed > > So, if we set the minimum compiler level for AIX to 17, we can remove the xlc > toolchain at all. > We cannot remove every reference to xlc, because at least some headers we > still use the xlc version (globalDefinitions_xlc.hpp) @JoKern65 Thanks for the explanation! It would be nice to clear out the xlc stuff. Let's open a separate issue for that once this is integrated. I also believe that the selection of `globalDefinitions_xlc.hpp` is done by #ifdefs in Hotspot source code, and has no relation to the build systems concept of toolchains. - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1945927380
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Thu, 15 Feb 2024 10:40:53 GMT, Magnus Ihse Bursie wrote: > What does this mean? That you are not using xlc at all? Or is it clang but > still with an xlc frontend, so all xlc flags etc need to stay? The `xlc` toolchain is for the compiler versions up to 16 (xlclang++); the `clang` toolchain is for the compiler versions 17 (ibm-clang++_r) and higher. For the 17 Compiler the frontend is `clang`-ish and we are using the `clang` flags instead of the `xlc` flags. `toolchain.m4` decides on the basis of the found compiler which toolchain to use as default. # On AIX the default toolchain depends on the installed (found) compiler # xlclang++ -> xlc toolchain # ibm-clang++_r -> clang toolchain # The compiler is searched on the PATH and TOOLCHAIN_PATH # xlclang++ has precedence over ibm-clang++_r if both are installed So, if we set the minimum compiler level for AIX to 17, we can remove the xlc toolchain at all. We cannot remove every reference to xlc, because at least some headers we still use the xlc version (globalDefinitions_xlc.hpp) - PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1945873440
Re: RFR: 8325878: Require minimum Clang version 13
On Thu, 15 Feb 2024 05:19:45 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of Clang > to be used for building OpenJDK from 3.5 to 13. > > This permits enabling C++17 (JDK-8314488), though Clang 5 might suffice for > that. A minimum of Clang 13 also obtains a critical bug fix for the > [[noreturn]] > attribute (see JDK-8303805). > > Testing: mach5 tier1, which includes building with a recent version of > Xcode/clang. Change looks fine from a build perspective. - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17862#pullrequestreview-1882418401
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. Change look fine from a build perspective. What does this mean? That you are not using xlc at all? Or is it clang but still with an xlc frontend, so all xlc flags etc need to stay? - Marked as reviewed by ihse (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17857#pullrequestreview-1882416313 PR Comment: https://git.openjdk.org/jdk/pull/17857#issuecomment-1945809787
Integrated: 8325877: Split up NativeCompilation.gmk
On Wed, 14 Feb 2024 15:33:22 GMT, Magnus Ihse Bursie wrote: > The file NativeCompilation.gmk is a beast. It is one of the largest in the > build system, and it is not very well organized. This makes it hard to read, > understand, debug, edit and modify, especially since IDEs have a hard time > helping out with makefiles, so you get very little overview or navigation > support. > > This patch will split up the file into several parts. The splits are somewhat > arbitrary, but tries to keep things sort of logically connected, and make the > chunks somewhat approximate equal size. > > I've gone to great pains to make sure I do not accidentally change anything. > The order for the code in each of these files are the same as in the original > NativeCompilation.gmk. I have not rearranged any code (with a few trivially > exceptions, moving some assignments to allow better grouping), and instead > preferred to split up functionality in several parts (as in > SetupBasicVariables1-3). > > Since I include the files in alphabetic order, some code will inevitable > switch places, but this should either be just defines (which are trivially > safe to move around), or it should be code that are independent of each other. > > My intention is to follow up this shuffling with more intrusive fixes, that > can e.g. reorder stuff to make for more logical grouping. But I want to do > that separately. This pull request has now been integrated. Changeset: 0d51b769 Author:Magnus Ihse Bursie URL: https://git.openjdk.org/jdk/commit/0d51b76947324643166cdaf9ca703431bd83bc0e Stats: 2541 lines in 7 files changed: 1418 ins; 1101 del; 22 mod 8325877: Split up NativeCompilation.gmk Reviewed-by: erikj, jwaters - PR: https://git.openjdk.org/jdk/pull/17849
Re: RFR: 8325877: Split up NativeCompilation.gmk [v2]
On Thu, 15 Feb 2024 08:53:35 GMT, Julian Waters wrote: > This is not going to be fun to rebase on top on in my port :( Apologies. :( But I think it might not be that hard either, if you do it correctly. That was one of my goals by keeping the order so strict, to facilitate this kind of merges. Basically, you can consider each of the new files as a copy of the original NativeCompilation.gmk. So take your version of NativeCompilation.gmk, and copy it once for each of the new files. And then if you use a good diff tool (personally, I prefer Meld), it will show the "deleted" parts, and you can just delete them, and any changes you have done to individual lines will be kept in the "saved" parts. This assumes that you have only made a bit of change here and there; if you have reordered stuff or whatever, then you're in trouble. I hope you understand what I mean; ask if I did not express myself clearly enough. - PR Comment: https://git.openjdk.org/jdk/pull/17849#issuecomment-1945800763
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17857#pullrequestreview-1882190279
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. make/autoconf/toolchain.m4 line 56: > 54: TOOLCHAIN_MINIMUM_VERSION_gcc="6.0" > 55: TOOLCHAIN_MINIMUM_VERSION_microsoft="19.28.0.0" # VS2019 16.8, aka MSVC > 14.28 > 56: TOOLCHAIN_MINIMUM_VERSION_xlc="17.1.1.4" We do not build AIX with the xlc toolchain any more but the clang one. So this line only stops a build if someone is trying to build with xlc 16 against toolchain xlc. I have to agree to @TheRealMDoerr, that the correct change would be to remove the xlc toolchain in jdk23 at all. - PR Review Comment: https://git.openjdk.org/jdk/pull/17857#discussion_r1490650675
Re: RFR: 8325877: Split up NativeCompilation.gmk [v2]
On Wed, 14 Feb 2024 15:44:27 GMT, Magnus Ihse Bursie wrote: >> The file NativeCompilation.gmk is a beast. It is one of the largest in the >> build system, and it is not very well organized. This makes it hard to read, >> understand, debug, edit and modify, especially since IDEs have a hard time >> helping out with makefiles, so you get very little overview or navigation >> support. >> >> This patch will split up the file into several parts. The splits are >> somewhat arbitrary, but tries to keep things sort of logically connected, >> and make the chunks somewhat approximate equal size. >> >> I've gone to great pains to make sure I do not accidentally change anything. >> The order for the code in each of these files are the same as in the >> original NativeCompilation.gmk. I have not rearranged any code (with a few >> trivially exceptions, moving some assignments to allow better grouping), and >> instead preferred to split up functionality in several parts (as in >> SetupBasicVariables1-3). >> >> Since I include the files in alphabetic order, some code will inevitable >> switch places, but this should either be just defines (which are trivially >> safe to move around), or it should be code that are independent of each >> other. >> >> My intention is to follow up this shuffling with more intrusive fixes, that >> can e.g. reorder stuff to make for more logical grouping. But I want to do >> that separately. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Remove debug code (and fix one more space) > - Restore mistakenly deleted space This is not going to be fun to rebase on top on in my port :( - Marked as reviewed by jwaters (Committer). PR Review: https://git.openjdk.org/jdk/pull/17849#pullrequestreview-1882142178
Re: RFR: 8325880: Require minimum Open XL C/C++ version 17.1.1
On Wed, 14 Feb 2024 22:43:37 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of IBM > Open XL C/C++. SAP dropped support for older versions in JDK 22, only > supporting the version specified in this change. > > I need someone from the aix-ppc porters to test and review the change. LGTM and works for us. @JoKern65: We should probably remove the old build pipeline after this is integrated. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17857#pullrequestreview-1882123803
Re: RFR: 8325878: Require minimum Clang version 13
On Thu, 15 Feb 2024 05:19:45 GMT, Kim Barrett wrote: > Please review this change that updates the minimum supported version of Clang > to be used for building OpenJDK from 3.5 to 13. > > This permits enabling C++17 (JDK-8314488), though Clang 5 might suffice for > that. A minimum of Clang 13 also obtains a critical bug fix for the > [[noreturn]] > attribute (see JDK-8303805). > > Testing: mach5 tier1, which includes building with a recent version of > Xcode/clang. Marked as reviewed by jwaters (Committer). - PR Review: https://git.openjdk.org/jdk/pull/17862#pullrequestreview-1882097398
