JDK-8266748: Move modifiers code to Signatures.java #4142

hns · 2021-05-21T08:46:48Z

This change consolidates the code to generate type signature modifiers into Signatures.TypeSignature.

Although this mostly consists of moving the code from ClassWriterImpl and Utils to Signatures, I also avoided the need to split the modifiers string when processing preview modifiers by returning a List<String> instead of a String in what used to be Utils.modifiersToString and is now TypeSignature.getModifiers.

Progress

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

Issue

JDK-8266748: Move modifiers code to Signatures.java

Reviewers

Jonathan Gibbons (@jonathan-gibbons - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4142

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

Using diff file

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

bridgekeeper · 2021-05-21T08:47:56Z

👋 Welcome back hannesw! 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-05-21T08:50:05Z

@hns 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-05-21T08:56:52Z

Webrevs

01: Full - Incremental (b5c5a95c)
00: Full (9970cd2e)

jonathan-gibbons

General comments; not a final review.

Moving code out of Utils is generally always good but it's a bit of a shame to move it down into formats.html. That being said, Signatures is a good abstraction to be building.
The medium amount of use of HtmlDocletWriter in Signatures is a code-smell, although arguably, we're just exposing an existing code-smell. We should (generally) continue our efforts to move code out of HtmlDocletWriter into other more-focussed abstractions.

jonathan-gibbons

Minor changes suggestion, to access items from the configuration where possible.

As a followup, it would be good to look at the remaining uses of HtmlDocletWriter in Signatures. There is at least a theme of "methods to create links", and if those methods are just used in the Signatures class, they would be candidates to move into that class as well.

jonathan-gibbons · 2021-06-03T17:16:29Z

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

            if (!annotationInfo.isEmpty()) {
                content.add(HtmlTree.SPAN(HtmlStyle.annotations, annotationInfo));
            }
            content.add(HtmlTree.SPAN(HtmlStyle.modifiers, modifiers));

            HtmlTree nameSpan = new HtmlTree(TagName.SPAN).setStyle(HtmlStyle.elementName);
            Content className = Text.of(utils.getSimpleName(typeElement));
-            if (classWriter.options.linkSource()) {
-                classWriter.addSrcLink(typeElement, className, nameSpan);
+            if (writer.options.linkSource()) {


better to use configuration.getOptions()

jonathan-gibbons · 2021-06-03T17:17:44Z

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

                            HtmlLinkInfo.Kind.PERMITTED_SUBCLASSES,
                            type));
                    permitsSpan.add(link);
                }
                if (linkablePermits.size() < permits.size()) {
-                    Content c = Text.of(classWriter.resources.getText("doclet.not.exhaustive"));
+                    Content c = Text.of(writer.resources.getText("doclet.not.exhaustive"));


Better to use configuration.getDocResources()

jonathan-gibbons · 2021-06-03T17:19:05Z

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

+                             writer.htmlIds.forPreviewSection(typeElement),
+                             writer.contents.previewMark)));


use the configuration for these two values

hns · 2021-06-07T13:27:10Z

Minor changes suggestion, to access items from the configuration where possible.

As a followup, it would be good to look at the remaining uses of HtmlDocletWriter in Signatures. There is at least a theme of "methods to create links", and if those methods are just used in the Signatures class, they would be candidates to move into that class as well.

I changed the mentioned accesses to use configuration instead of writer.

Most of the link methods in HtmlDocletWriter are used all over the place. addSrcLink is only used in TypeSignature and MemberSignature, but there's various circumstances that makes a move to Signatures unattractive (it accesses fields in HtmlDocletWriter, to which static nested classes TypeSignature and MemberSignature don't share a common reference).

jonathan-gibbons

Generally good, and as with many cleanups, it's a great step in the right direction. As I indicated in a previous round of review, I think that further cleanup is possible (later) by moving more code out of HtmlDocletWriter into more specific abstractions, possibly including this one.

openjdk · 2021-06-08T16:27:23Z

@hns 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:

8266748: Move modifiers code to Signatures.java

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 259 new commits pushed to the master branch:

159cb6f: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
8158b82: 8268327: Upstream: 8268169: The system lookup can not find stdio functions such as printf on Windows 10
6843576: 8268227: java/foreign/TestUpcall.java still times out
2717fcb: 8232948: javac -h should mangle the overload argument signature
89da202: 8266159: macOS ARM + Metal pipeline shows artifacts on Swing Menu with Java L&F
61ab4b9: 8267564: JDK-8252971 causes SPECjbb2015 socket exceptions on Windows when MKS is installed
00c88f7: 8266918: merge_stack in check_code.c add NULL check
8105478: 8268165: AsyncLogging will crash if rotate() fails
fd91b2a: 8265440: IGV: make node selection more visible
81bad59: 8257774: G1: Trigger collect when free region count drops below threshold to prevent evacuation failures
... and 249 more: https://git.openjdk.java.net/jdk/compare/9eaa4afc99b09f4704e4d641f95104be40b9ea66...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.

hns · 2021-06-08T19:19:57Z

/integrate

openjdk · 2021-06-08T19:21:42Z

@hns Since your change was applied there have been 264 commits pushed to the master branch:

4dd0e7e: 8259806: Clean up terminology on the "All Classes" page
dc6c96b: 8263468: New page for "recent" new API
fafc4d9: 8268352: Rename javadoc Messager class to JavadocLog
b568e87: 8237388: serviceability/dcmd/framework/VMVersionTest.java fails with connection refused error.
c21cc93: 8248877: Document API contract for MetaspaceObj subtypes
159cb6f: 8268083: JDK-8267706 breaks bin/idea.sh on a Mac
8158b82: 8268327: Upstream: 8268169: The system lookup can not find stdio functions such as printf on Windows 10
6843576: 8268227: java/foreign/TestUpcall.java still times out
2717fcb: 8232948: javac -h should mangle the overload argument signature
89da202: 8266159: macOS ARM + Metal pipeline shows artifacts on Swing Menu with Java L&F
... and 254 more: https://git.openjdk.java.net/jdk/compare/9eaa4afc99b09f4704e4d641f95104be40b9ea66...master

Your commit was automatically rebased without conflicts.

Pushed as commit f9b593d.

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

hns added 2 commits May 21, 2021 10:19

JDK-8266748: Move modifiers code to Signatures.java

65a635f

JDK-8266748: Make getModifiers() non-static

9970cd2

hns changed the title ~~Jdk 8266748~~ Jdk 8266748: Move modifiers code to Signatures.java May 21, 2021

openjdk bot added the javadoc javadoc-dev@openjdk.org label May 21, 2021

hns changed the title ~~Jdk 8266748: Move modifiers code to Signatures.java~~ JDK-8266748: Move modifiers code to Signatures.java May 21, 2021

openjdk bot added the rfr Pull request is ready for review label May 21, 2021

jonathan-gibbons reviewed May 28, 2021

View reviewed changes

jonathan-gibbons suggested changes Jun 3, 2021

View reviewed changes

JDK-8266748: Use configuration instead of writer where possible

b5c5a95

jonathan-gibbons approved these changes Jun 8, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 8, 2021

openjdk bot closed this Jun 8, 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 Jun 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8266748: Move modifiers code to Signatures.java #4142

JDK-8266748: Move modifiers code to Signatures.java #4142

hns commented May 21, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented May 21, 2021

openjdk bot commented May 21, 2021

mlbridge bot commented May 21, 2021 •

edited

Loading

jonathan-gibbons left a comment

jonathan-gibbons left a comment

jonathan-gibbons Jun 3, 2021

jonathan-gibbons Jun 3, 2021

jonathan-gibbons Jun 3, 2021

hns commented Jun 7, 2021

jonathan-gibbons left a comment

openjdk bot commented Jun 8, 2021 •

edited

Loading

hns commented Jun 8, 2021

openjdk bot commented Jun 8, 2021

		writer.htmlIds.forPreviewSection(typeElement),
		writer.contents.previewMark)));

JDK-8266748: Move modifiers code to Signatures.java #4142

JDK-8266748: Move modifiers code to Signatures.java #4142

Conversation

hns commented May 21, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented May 21, 2021

openjdk bot commented May 21, 2021

mlbridge bot commented May 21, 2021 • edited Loading

Webrevs

jonathan-gibbons left a comment

Choose a reason for hiding this comment

jonathan-gibbons left a comment

Choose a reason for hiding this comment

jonathan-gibbons Jun 3, 2021

Choose a reason for hiding this comment

jonathan-gibbons Jun 3, 2021

Choose a reason for hiding this comment

jonathan-gibbons Jun 3, 2021

Choose a reason for hiding this comment

hns commented Jun 7, 2021

jonathan-gibbons left a comment

Choose a reason for hiding this comment

openjdk bot commented Jun 8, 2021 • edited Loading

hns commented Jun 8, 2021

openjdk bot commented Jun 8, 2021

hns commented May 21, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented May 21, 2021 •

edited

Loading

openjdk bot commented Jun 8, 2021 •

edited

Loading