8265684: implement Sealed Classes as a standard feature in Java, javadoc changes #3613

vicente-romero-oracle · 2021-04-22T02:21:14Z

Please review the javadoc related changes to make sealed classes a final feature in Java. The patch is mostly removing preview features related code that is not necessary anymore in javadoc.

TIA

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8265684: implement Sealed Classes as a standard feature in Java, javadoc changes

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer) ⚠️ Review applies to 875a7ed

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3613/head:pull/3613
$ git checkout pull/3613

Update a local copy of the PR:
$ git checkout pull/3613
$ git pull https://git.openjdk.java.net/jdk pull/3613/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3613

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3613.diff

…doc changes

bridgekeeper · 2021-04-22T02:21:42Z

👋 Welcome back vromero! 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 · 2021-04-22T02:23:59Z

@vicente-romero-oracle The following label will be automatically applied to this pull request:

javadoc

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

mlbridge · 2021-04-22T02:27:31Z

Webrevs

jonathan-gibbons

Generally, I think you have have deleted too much.

Although the code will work as you propose, at some point there will be new preview language features, and I think it would be better to leave a skeleton of the code in place, even if the set of preview language features is empty for now.

I think it would be better to limit the deletions to those lines of code that are specific to sealed classes and permits.

jonathan-gibbons · 2021-04-22T21:10:50Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java

-    private static final Set<String> previewModifiers
-            = Set.of("sealed", "non-sealed");
-


Rather than delete those lines, for future support it might be better just to set previewModifiers to Collections.emptySet()

jonathan-gibbons · 2021-04-22T21:11:30Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java

-            if (previewModifiers.contains(modifiersPart)) {
-                mods.add(modifiersPart);
-                mods.add(HtmlTree.SUP(links.createLink(htmlIds.forPreviewSection(typeElement),
-                                                       contents.previewMark)));
-            } else {
-                mods.add(modifiersPart);
-            }
+
+            mods.add(modifiersPart);


See previous comment, to leave previewModifiers in place as an empty set

jonathan-gibbons · 2021-04-22T21:13:49Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

    private List<Content> getPreviewNotes(TypeElement el) {
        String className = el.getSimpleName().toString();
        List<Content> result = new ArrayList<>();
        PreviewSummary previewAPITypes = utils.declaredUsingPreviewAPIs(el);
        Set<TypeElement> previewAPI = new HashSet<>(previewAPITypes.previewAPI);
        Set<TypeElement> reflectivePreviewAPI = new HashSet<>(previewAPITypes.reflectivePreviewAPI);
        Set<TypeElement> declaredUsingPreviewFeature = new HashSet<>(previewAPITypes.declaredUsingPreviewFeature);
-        Set<DeclarationPreviewLanguageFeatures> previewLanguageFeatures = new HashSet<>();


consider not deleting this; just leave it empty

jonathan-gibbons · 2021-04-22T21:14:36Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

-            if (previewLanguageFeatures.contains(DeclarationPreviewLanguageFeatures.SEALED_PERMITS)) {
-                previewLanguageFeatures.remove(DeclarationPreviewLanguageFeatures.SEALED);
-            }


These lines should go, because they specifically refer to SEALED stuff, but the rest of the block could stay

jonathan-gibbons · 2021-04-22T21:15:18Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java

-    private Content withPreviewFeatures(String key, String className, String featureName, List<String> features) {
-        String[] sep = new String[] {""};
-        ContentBuilder featureCodes = new ContentBuilder();
-        features.stream()
-                .forEach(c -> {
-                    featureCodes.add(sep[0]);
-                    featureCodes.add(HtmlTree.CODE(new ContentBuilder().add(c)));
-                    sep[0] = ", ";
-                });
-        return contents.getContent(key,
-                                   HtmlTree.CODE(Text.of(className)),
-                                   new HtmlTree(TagName.EM).add(featureName),
-                                   featureCodes);
-    }
-


Nothing here seems specific to SEALED, so leave it

jonathan-gibbons · 2021-04-22T21:16:59Z

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/Signatures.java

-                        Content link =
-                                classWriter.links.createLink(classWriter.htmlIds.forPreviewSection(typeElement),
-                                                             classWriter.contents.previewMark);
-                        permitsSpan.add(HtmlTree.SUP(link));


OK, yes, this is specific to permits, part of the sealed feature

jonathan-gibbons

Depending who goes first, there may be a merge conflict with removing some of
the @SuppressWarnings annotations.

jonathan-gibbons · 2021-04-30T03:33:57Z

test/langtools/jdk/javadoc/doclet/testSealedTypes/TestSealedTypes.java

    public void testPartialMultiplePermitsWithSubtypes1(Path base) throws IOException {
        Path src = base.resolve("src");
        tb.writeJavaFiles(src,
                "package p; public sealed class A permits B,C,D { }",
                "package p; public final  class B extends A { }",
                "package p; public final  class C extends A { }",
-                "package p;        sealed class D extends A permits D1, D2 { }",
+                "package p; sealed class D extends A permits D1, D2 { }",


(minor) don't delete the spaces; the class names were previously vertically aligned

sure, done, thank for the review

openjdk · 2021-04-30T03:35:49Z

@vicente-romero-oracle 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:

8265684: implement Sealed Classes as a standard feature in Java, javadoc changes

Reviewed-by: jjg

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

vicente-romero-oracle · 2021-05-20T00:39:58Z

/issue add

openjdk · 2021-05-20T00:40:26Z

@vicente-romero-oracle Command syntax:

/issue [add|remove] <id>[,<id>,...]
/issue [add] <id>: <description>
`/issue create [PX] /[subcomponent]

Some examples:

/issue add JDK-1234567,4567890
/issue remove JDK-4567890
/issue 1234567: Use this exact title
`/issue create hotspot/jfr
`/issue create P4 core-libs/java.nio

If issues are specified only by their ID, the title will be automatically retrieved from JBS. The project prefix (JDK- in the above examples) is optional. Separate multiple issue IDs using either spaces or commas.

vicente-romero-oracle · 2021-05-20T09:21:44Z

/integrate

openjdk · 2021-05-20T09:23:34Z

@vicente-romero-oracle Since your change was applied there have been 4 commits pushed to the master branch:

0fa9223: 8260517: implement Sealed Classes as a standard feature in Java
31b98e1: 8265319: implement Sealed Classes as a standard feature in Java, javax.lang.model changes
726785b: 8267155: runtime/os/TestTracePageSizes times out
8e3549f: 8266332: Adler32 intrinsic for x86 64-bit platforms

Your commit was automatically rebased without conflicts.

Pushed as commit 7dcb9fd.

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

8265684: implement Sealed Classes as a standard feature in Java, java…

2b7cee9

…doc changes

openjdk bot added the rfr Pull request is ready for review label Apr 22, 2021

openjdk bot added the javadoc javadoc-dev@openjdk.org label Apr 22, 2021

jonathan-gibbons reviewed Apr 22, 2021

View reviewed changes

vicente-romero-oracle added 5 commits April 22, 2021 17:29

Merge branch 'master' into JDK-8265684

7d0ba41

addressing review comments

dc1bd3f

additional commit addressing review comments

5d02ce5

Merge branch 'master' into JDK-8265684

5a139f5

addressing indentation issues

846d34b

jonathan-gibbons approved these changes Apr 30, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 30, 2021

vicente-romero-oracle added 2 commits April 30, 2021 13:36

Merge branch 'master' into JDK-8265684

436a75c

fixing indentation: review comment

688d564

hns mentioned this pull request May 6, 2021

JDK-8232644: bugs in serialized-form.html #3817

Closed

3 tasks

Merge branch 'master' into JDK-8265684

875a7ed

jonathan-gibbons approved these changes May 6, 2021

View reviewed changes

manual merge with master

bf1611c

openjdk bot closed this May 20, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 20, 2021

mkurz mentioned this pull request May 20, 2021

Create a Java 17 distribution jOOQ/jOOQ#11729

Closed

mkurz mentioned this pull request May 20, 2021

scala.tools.nsc.javac.JavaParsers can't parse JDK17 "Sealed Classes" scala/bug#12159

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8265684: implement Sealed Classes as a standard feature in Java, javadoc changes #3613

8265684: implement Sealed Classes as a standard feature in Java, javadoc changes #3613

vicente-romero-oracle commented Apr 22, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Apr 22, 2021

openjdk bot commented Apr 22, 2021

mlbridge bot commented Apr 22, 2021 •

edited

Loading

jonathan-gibbons left a comment •

edited

Loading

jonathan-gibbons Apr 22, 2021

jonathan-gibbons Apr 22, 2021

jonathan-gibbons Apr 22, 2021

jonathan-gibbons Apr 22, 2021

jonathan-gibbons Apr 22, 2021

jonathan-gibbons Apr 22, 2021

jonathan-gibbons left a comment

jonathan-gibbons Apr 30, 2021

vicente-romero-oracle Apr 30, 2021

openjdk bot commented Apr 30, 2021 •

edited

Loading

vicente-romero-oracle commented May 20, 2021

openjdk bot commented May 20, 2021

vicente-romero-oracle commented May 20, 2021

openjdk bot commented May 20, 2021

		private static final Set<String> previewModifiers
		= Set.of("sealed", "non-sealed");

8265684: implement Sealed Classes as a standard feature in Java, javadoc changes #3613

8265684: implement Sealed Classes as a standard feature in Java, javadoc changes #3613

Conversation

vicente-romero-oracle commented Apr 22, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 22, 2021

openjdk bot commented Apr 22, 2021

mlbridge bot commented Apr 22, 2021 • edited Loading

Webrevs

jonathan-gibbons left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathan-gibbons left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Apr 30, 2021 • edited Loading

vicente-romero-oracle commented May 20, 2021

openjdk bot commented May 20, 2021

vicente-romero-oracle commented May 20, 2021

openjdk bot commented May 20, 2021

vicente-romero-oracle commented Apr 22, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 22, 2021 •

edited

Loading

jonathan-gibbons left a comment •

edited

Loading

openjdk bot commented Apr 30, 2021 •

edited

Loading