Skip to content
This repository has been archived by the owner on Sep 19, 2023. It is now read-only.
/ jdk19 Public archive

JDK-8288058: Broken links on constant-values page #62

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Jun 23, 2022

JDK 19:

Please review a medium-simple fix for some broken links on the constant-values page, from the "contents" index at the top of the page to the summary tables lower down on the page.

There are two root causes:

The packages are grouped into "groups" with headings based on abbreviated package names, meaning "at most two leading components of each package name". At one point there is a probable typo, when a full package name is used instead of the abbreviated name, thus causing the wrong "id" to be generated, thus causing broken links. The fix for this is obvious: use the correct name.

More seriously, the abbreviated package name is modeled using a PackageElement. While seemingly a good idea, the construction of these elements inherits the module element of the originating package. Then, the package elements are used to build a Set of headings, based on these package elements. The problem is that when the subpackages are found in different modules, they give rise to distinct "abbreviated package elements" in different modules and thus cause repeated instances of like-named headers in the output. This is easily visible as repeated headers for com.sun.* in (for example) the JDK 18 API. The fix is to use a Set to model the set of headers and associated links, instead of Set.

Along with the two fixes described above, there is minor code cleanup, including the removal of some ill-considered code in both Workarounds and Utils. Apart from those classes, the changes are confined to the "constant summary" builder and writers. The use of a member called "first" and related shenanigans is a holdover from the days when the page was generated literally top-to-bottom with print statements. It is easily removed.

The only existing test for the constant values page was laughably insufficient, and was effectively just a regression test for a minor issue in JDK 1.4.2. The test is enhanced with a bunch of new test cases, that exercise different scenarios for the list of contents at the top of the page, and the links to the corresponding sections lower down. The new test cases leverage the implicit support for checking links in the generated output, although they do forcibly enable that support, just to make sure it is active.

When comparing the new JDK API docs against recent JDK API docs, the only changes are the expected changes in the links and headings on the constant-values.html page.

There is more cleanup that could be done to this page ... such as suppressing the "contents:" list when it is just a single entry ... but that is an enhancement that is out of scope for this bug fix.


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

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 62

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/62.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 23, 2022

👋 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 openjdk bot added the rfr Pull request is ready for review label Jun 23, 2022
@openjdk
Copy link

openjdk bot commented Jun 23, 2022

@jonathan-gibbons 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.

@openjdk openjdk bot added the javadoc javadoc-dev@openjdk.org label Jun 23, 2022
@mlbridge
Copy link

mlbridge bot commented Jun 23, 2022

Webrevs

Copy link
Member

@pavelrappo pavelrappo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why javadoc has been abbreviating packages down to two name components; Jon, do you have any ideas on that?

String parsedPackageName = utils.parsePackageName(pkg);
Content packageNameContent = Text.of(parsedPackageName + ".*");
link = links.createLink(DocLink.fragment(parsedPackageName),
Content packageNameContent = Text.of(abbrevPackageName + ".*");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Separately. This is somewhat surprising to see that javadoc has been using .* to denote "subpackages" (or "related packages" as we call them elsewhere), whereas Java import statements use .* to denote immediately enclosed classes and interfaces, or static members thereof.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably so, but that's a different issue.

*/
void addLinkToPackageContent(PackageElement pkg, Set<PackageElement> writtenPackageHeaders,
Content content);
void addLinkToPackageContent(String abbrevPackageName, Content content);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels strange to downgrade a package representation from PackageElement to String. Especially considering that this interface expresses other program elements through javax.lang.model.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it may seem strange. Two points:

  1. There is no representation for a package that ignores the enclosing module
  2. These are not packages so much as headings, for the purpose of provides a "Contents" list at the head of the page, for ease of navigation.

@openjdk
Copy link

openjdk bot commented Jun 28, 2022

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

8288058: Broken links on constant-values page

Reviewed-by: prappo

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

  • adbd200: 8289228: SegmentAllocator::allocateArray null handling is too lax
  • b449038: 8288445: AArch64: C2 compilation fails with guarantee(!true || (true && (shift != 0))) failed: impossible encoding
  • 2efa89a: 8289251: ProblemList java/lang/ref/OOMEInReferenceHandler.java
  • 17ef8ca: 8288524: Allow @systemProperty to appear in overview documentation
  • caa6b74: 8289240: ProblemList java/lang/reflect/callerCache/ReflectionCallerCacheTest.java in -Xcomp mode
  • 28913f4: 8289235: ProblemList vmTestbase/nsk/jdi/ClassType/invokeMethod/invokemethod011/TestDescription.java when run with vthread wrapper
  • 7746664: 8280826: Document set of strings javac recognizes for SuppressWarnings
  • 2c8ada6: 8289188: SegmentAllocator:allocateArray(*) default behavior mismatch to spec
  • 699ad45: 8289093: BlockLocationPrinter fails to decode addresses with G1
  • 784a0f0: 8288683: C2: And node gets wrong type due to not adding it back to the worklist in CCP
  • ... and 7 more: https://git.openjdk.org/jdk19/compare/1f9521e6cb2f701f8712b4ec941ff1dbb45dad4e...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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jun 28, 2022
@jonathan-gibbons
Copy link
Contributor Author

/integrate

@openjdk
Copy link

openjdk bot commented Jun 28, 2022

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

  • a814293: 8275784: Bogus warning generated for record with compact constructor
  • 6f9717b: 8288836: (fs) Files.writeString spec for IOException has "specified charset" when no charset is provided
  • 9048cef: 8288425: Footprint regression due MH creation when initializing StringConcatFactory
  • adbd200: 8289228: SegmentAllocator::allocateArray null handling is too lax
  • b449038: 8288445: AArch64: C2 compilation fails with guarantee(!true || (true && (shift != 0))) failed: impossible encoding
  • 2efa89a: 8289251: ProblemList java/lang/ref/OOMEInReferenceHandler.java
  • 17ef8ca: 8288524: Allow @systemProperty to appear in overview documentation
  • caa6b74: 8289240: ProblemList java/lang/reflect/callerCache/ReflectionCallerCacheTest.java in -Xcomp mode
  • 28913f4: 8289235: ProblemList vmTestbase/nsk/jdi/ClassType/invokeMethod/invokemethod011/TestDescription.java when run with vthread wrapper
  • 7746664: 8280826: Document set of strings javac recognizes for SuppressWarnings
  • ... and 10 more: https://git.openjdk.org/jdk19/compare/1f9521e6cb2f701f8712b4ec941ff1dbb45dad4e...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Jun 28, 2022
@openjdk openjdk bot closed this Jun 28, 2022
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 28, 2022
@openjdk
Copy link

openjdk bot commented Jun 28, 2022

@jonathan-gibbons Pushed as commit c42b796.

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

@jonathan-gibbons jonathan-gibbons deleted the 8288058.constant-value-links-2 branch June 28, 2022 15:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
integrated Pull request has been integrated javadoc javadoc-dev@openjdk.org
2 participants