Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8157682: @inheritDoc doesn't work with @exception #2818

Closed

Conversation

@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Mar 3, 2021

Please review a fix to improve the handling of the legacy @exception tag.

A patch for this was previously submitted. However, that patch introduced an explicit new ExceptionTaglet class, whereas the fix here just reuses the existing ThrowsTaglet, registering it under an additional name.

The test in the original patch is enhanced to check all 4 combinations of (throws, exception) in the base class and (throws, exception) in the subclass.


Progress

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

Issue

Reviewers

Contributors

  • Yano, Masanori <yano-masanori@jp.fujitsu.com>
  • Jonathan Gibbons <jjg@openjdk.org>

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2818/head:pull/2818
$ git checkout pull/2818

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 3, 2021

👋 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.

Loading

@openjdk openjdk bot added the rfr label Mar 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 3, 2021

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

Loading

@openjdk openjdk bot added the javadoc label Mar 3, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 3, 2021

Loading

Copy link
Member

@pavelrappo pavelrappo left a comment

This commit changes the order of block tags in the output. The fact that no tests failed when I ran them surprised me. I think we should both fix that order and introduce a test for it.

Loading

Map<String, Taglet> taglets = new TreeMap<String, Taglet>();
taglets.putAll(allTaglets);
Copy link
Member

@pavelrappo pavelrappo Mar 4, 2021

Choose a reason for hiding this comment

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

Consider squashing these two lines into one:

Map<String, Taglet> taglets = new TreeMap<>(allTaglets);

and deleting this now unused import:

import java.util.Comparator;

Loading

blockTagletsByLocation.get(l).add(t);
List<Taglet> list = blockTagletsByLocation.get(l);
if (!list.contains(t)) {
list.add(t);
}
Copy link
Member

@pavelrappo pavelrappo Mar 4, 2021

Choose a reason for hiding this comment

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

I suppose the purpose of that is to filter out a now duplicated ThrowsTaglet. We should separately consider using a more appropriate data structure there.

Loading

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Mar 4, 2021

Choose a reason for hiding this comment

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

yeah, but ... this is just one-off init ... it's the "standard" problem of list vs. sets, and list-y sets vs. set-y lists.

Loading

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Mar 4, 2021

/contributor add Yano, Masanori yano-masanori@jp.fujitsu.com

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 4, 2021

@jonathan-gibbons
Contributor Yano, Masanori <yano-masanori@jp.fujitsu.com> successfully added.

Loading

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Mar 4, 2021

This commit changes the order of block tags in the output. The fact that no tests failed when I ran them surprised me. I think we should both fix that order and introduce a test for it.

Can you give more details? Did you observe that anywhere, or are you inferring that from the code somewhere?

Loading

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Mar 4, 2021

/contributor add jonathan-gibbons

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 4, 2021

@jonathan-gibbons Could not parse jonathan-gibbons as a valid contributor.
Syntax: /contributor (add|remove) [@user | openjdk-user | Full Name <email@address>]. For example:

  • /contributor add @openjdk-bot
  • /contributor add duke
  • /contributor add J. Duke <duke@openjdk.org>

Loading

@pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Mar 4, 2021

This commit changes the order of block tags in the output. The fact that no tests failed when I ran them surprised me. I think we should both fix that order and introduce a test for it.

Can you give more details? Did you observe that anywhere, or are you inferring that from the code somewhere?

I saw that difference in the produced documentation for JDK. I suggest you build documentation for "before" and "after", each time with the -notimestamp option, and compare them; you should see what I saw.

Loading

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Mar 4, 2021

/contributor add @jonathan-gibbons

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 4, 2021

@jonathan-gibbons
Contributor Jonathan Gibbons <jjg@openjdk.org> successfully added.

Loading

@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Mar 4, 2021

This commit changes the order of block tags in the output. The fact that no tests failed when I ran them surprised me. I think we should both fix that order and introduce a test for it.

Can you give more details? Did you observe that anywhere, or are you inferring that from the code somewhere?

I saw that difference in the produced documentation for JDK. I suggest you build documentation for "before" and "after", each time with the -notimestamp option, and compare them; you should see what I saw.

Wow. Good catch! That was unexpected! Will investigate!

Loading

Copy link
Member

@pavelrappo pavelrappo left a comment

I can see that you propose to add a test for the output order of block tags separately: https://git.openjdk.java.net/jdk/pull/2835 (JDK-8263043).

Looks good.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2021

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

8157682: @inheritDoc doesn't work with @exception

Co-authored-by: Yano, Masanori <yano-masanori@jp.fujitsu.com>
Co-authored-by: Jonathan Gibbons <jjg@openjdk.org>
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 31 new commits pushed to the master branch:

  • 9730266: 8262973: Verify ParCompactionManager instance in PCAdjustPointerClosure
  • d91550e: 8262998: Vector API intrinsincs should not modify IR when bailing out
  • 80182f9: 8260925: HttpsURLConnection does not work with other JSSE provider.
  • dbef0ec: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
  • ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
  • 351889f: 8262508: Vector API's ergonomics is incorrect
  • 718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
  • c8b23e2: 8262064: Make compiler/ciReplay tests ignore lambdas in compilation replay
  • 02fbcb5: 8261532: Archived superinterface class cannot be accessed
  • 109af7b: 8261518: jpackage looks for main module in current dir when there is no module-path
  • ... and 21 more: https://git.openjdk.java.net/jdk/compare/2d2ef08c0fd99f8d486e47be96fb1559140c6bb3...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.

Loading

@openjdk openjdk bot added the ready label Mar 5, 2021
@jonathan-gibbons
Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons commented Mar 5, 2021

/integrate

Loading

@openjdk openjdk bot closed this Mar 5, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 5, 2021
@jonathan-gibbons jonathan-gibbons deleted the inheritDoc-exception branch Mar 5, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 5, 2021

@jonathan-gibbons Since your change was applied there have been 34 commits pushed to the master branch:

  • 8c13d26: 8263050: move HtmlDocletWriter.verticalSeparator to IndexWriter
  • 8d3de4b: 8262844: (fs) FileStore.supportsFileAttributeView might return false negative in case of ext3
  • 75fb7cc: 8259228: Zero: rewrite (put|get)field from if-else chains to switches
  • 9730266: 8262973: Verify ParCompactionManager instance in PCAdjustPointerClosure
  • d91550e: 8262998: Vector API intrinsincs should not modify IR when bailing out
  • 80182f9: 8260925: HttpsURLConnection does not work with other JSSE provider.
  • dbef0ec: 6323374: (coll) Optimize Collections.unmodifiable* and synchronized*
  • ee09bad: 8262300: jpackage app-launcher fails on linux when using JDK11 based runtime
  • 351889f: 8262508: Vector API's ergonomics is incorrect
  • 718d4d4: 8262989: Vectorize VectorShuffle checkIndexes, wrapIndexes and laneIsValid methods
  • ... and 24 more: https://git.openjdk.java.net/jdk/compare/2d2ef08c0fd99f8d486e47be96fb1559140c6bb3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 9755782.

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants