JDK-8301201: Allow \n@ inside inline tags using inlineContent #12264

jonathan-gibbons · 2023-01-27T18:41:45Z

Please review a simple change to allow the use of newline whitespace @ inside inline tags that allow rich content (that is, those parsed with inlineContent) as compared to those that only allow plain text (that is, those parsed with inlineText).

The fix is to delete the code which recognizes @ as the beginning of a block tag. Compare to the similar fix in JDK-8241780

The general TagTest.java is updated for the new feature.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8301201: Allow \n@ inside inline tags using inlineContent

Reviewers

Hannes Wallnöfer (@hns - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12264/head:pull/12264
$ git checkout pull/12264

Update a local copy of the PR:
$ git checkout pull/12264
$ git pull https://git.openjdk.org/jdk pull/12264/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 12264

View PR using the GUI difftool:
$ git pr show -t 12264

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12264.diff

bridgekeeper · 2023-01-27T18:43:32Z

👋 Welcome back jjg! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-01-27T18:45:44Z

@jonathan-gibbons The following labels will be automatically applied to this pull request:

compiler
javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-01-27T18:48:56Z

Webrevs

01: Full - Incremental (6b22d92f)
00: Full (a27b09b1)

hns · 2023-01-31T14:55:34Z

test/langtools/tools/javac/doctree/DocCommentTester.java

@@ -919,7 +919,7 @@ void check(TreePath path, Name name) throws Exception {
        String normalize(String s) {
            String s2 = s.trim().replaceFirst("\\.\\s*\\n *@", ".\n@");
            StringBuilder sb = new StringBuilder();
-            Pattern p = Pattern.compile("\\{@(code|literal)( )?");
+            Pattern p = Pattern.compile("(?i)\\{@([a-z][a-z0-9.:-]*)( )?");


Why is this change necessary? I see it is intended to impact the unknown inline tag in TagTest.java, but couldn't the test be written without this change?

Previously, the only tags in which newline at was supported were {@code} {@literal} and {@snippet} ... and this tag predated {@snippet} which is the main reason it's not included here.

Now newline at is supported in all inline tags, so the scope of this check is broadened accordingly.

hns · 2023-01-31T15:28:42Z

There's something wrong with this, several javac/doctree tests are failing.

jonathan-gibbons · 2023-02-01T00:14:44Z

There's something wrong with this, several javac/doctree tests are failing.

Yes, now fixed. The breakage was obviously due to the change you mentioned above (for code|literal tags). The fix in most cases (one exception) was to make the PrettyChecker more robust. The one difficult case was a syntactically invalid tag (which was in most ways a reasonable test case) but which led to false positive matches in the normalize code. For that case, I created a "backdoor" to pass a boolean flag to the normalize code by means of an annotation on the test case. Note that because the test case is only parsed and never analyzed, the annotation does not need a declaration, it's enough to have an annotation show up in the AST, where it can be accessed by the PrettyChecker code.

hns

Thanks for the extended comment for the normalize method. Looks good to me.

openjdk · 2023-02-01T16:43:01Z

@jonathan-gibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8301201: Allow \n@ inside inline tags using inlineContent

Reviewed-by: hannesw

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 89 new commits pushed to the master branch:

bc750f7: 8294527: Some java.security.debug options missing from security docs
7c6a8db: 8301447: [REDO] CodeHeap has virtual methods that are not overridden
cae577a: 8295486: Inconsistent constant field values observed during compilation
969f6a3: 8301093: C2 fails assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out
2a8ae2f: 8300256: C2: vectorization is sometimes skipped on loops where it would succeed
ef0d0a7: 8301402: os::print_location gets is_global_handle assert
4f6f3cc: 8301446: Remove unused includes of gc/shared/genOopClosures
a0aed9b: 8301459: Serial: Merge KeepAliveClosure into FastKeepAliveClosure
d269ebb: 8301570: Test runtime/jni/nativeStack/ needs to detach the native thread
8164cfb: 8300696: [AIX] AttachReturnError fails
... and 79 more: https://git.openjdk.org/jdk/compare/7eff578768454445a937c1569abab3d9b06c1313...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jonathan-gibbons · 2023-02-01T18:26:50Z

@hns Thanks for the reviews.

jonathan-gibbons · 2023-02-01T18:26:58Z

/integrate

openjdk · 2023-02-01T18:28:51Z

Going to push as commit 24ff3da.
Since your change was applied there have been 90 commits pushed to the master branch:

3361a26: 8298874: Update TestAllSuites.java for TLS v1.2 and 1.3
bc750f7: 8294527: Some java.security.debug options missing from security docs
7c6a8db: 8301447: [REDO] CodeHeap has virtual methods that are not overridden
cae577a: 8295486: Inconsistent constant field values observed during compilation
969f6a3: 8301093: C2 fails assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out
2a8ae2f: 8300256: C2: vectorization is sometimes skipped on loops where it would succeed
ef0d0a7: 8301402: os::print_location gets is_global_handle assert
4f6f3cc: 8301446: Remove unused includes of gc/shared/genOopClosures
a0aed9b: 8301459: Serial: Merge KeepAliveClosure into FastKeepAliveClosure
d269ebb: 8301570: Test runtime/jni/nativeStack/ needs to detach the native thread
... and 80 more: https://git.openjdk.org/jdk/compare/7eff578768454445a937c1569abab3d9b06c1313...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-02-01T18:29:13Z

@jonathan-gibbons Pushed as commit 24ff3da.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8301201: Allow \n@ inside inline tags using inlineContent

a27b09b

openjdk bot added the rfr label Jan 27, 2023

openjdk bot added javadoc compiler labels Jan 27, 2023

hns reviewed Jan 31, 2023

View reviewed changes

fix broken tests

6b22d92

hns approved these changes Feb 1, 2023

View reviewed changes

openjdk bot added the ready label Feb 1, 2023

openjdk bot added the integrated label Feb 1, 2023

openjdk bot closed this Feb 1, 2023

openjdk bot removed ready rfr labels Feb 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8301201: Allow \n@ inside inline tags using inlineContent #12264

JDK-8301201: Allow \n@ inside inline tags using inlineContent #12264

jonathan-gibbons commented Jan 27, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

mlbridge bot commented Jan 27, 2023 •

edited

Loading

hns Jan 31, 2023

jonathan-gibbons Feb 1, 2023

hns commented Jan 31, 2023

jonathan-gibbons commented Feb 1, 2023

hns left a comment

openjdk bot commented Feb 1, 2023

jonathan-gibbons commented Feb 1, 2023

jonathan-gibbons commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

JDK-8301201: Allow \n@ inside inline tags using inlineContent #12264

JDK-8301201: Allow \n@ inside inline tags using inlineContent #12264

Conversation

jonathan-gibbons commented Jan 27, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 27, 2023

openjdk bot commented Jan 27, 2023

mlbridge bot commented Jan 27, 2023 • edited Loading

Webrevs

hns Jan 31, 2023

Choose a reason for hiding this comment

jonathan-gibbons Feb 1, 2023

Choose a reason for hiding this comment

hns commented Jan 31, 2023

jonathan-gibbons commented Feb 1, 2023

hns left a comment

Choose a reason for hiding this comment

openjdk bot commented Feb 1, 2023

jonathan-gibbons commented Feb 1, 2023

jonathan-gibbons commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

openjdk bot commented Feb 1, 2023

jonathan-gibbons commented Jan 27, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 27, 2023 •

edited

Loading