8387417: [lworld] API doc improvements for java.lang.Object#2588
8387417: [lworld] API doc improvements for java.lang.Object#2588AlanBateman wants to merge 9 commits into
Conversation
|
👋 Welcome back alanb! A progress list of the required criteria for merging this PR into |
|
@AlanBateman 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: 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 5 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@liach) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Webrevs
|
| * {@link IdentityException}. | ||
| * When preview features are enabled, subclasses of {@code java.lang.Object} | ||
| * are either {@linkplain Class#isValue value classes} or identity classes. | ||
| * A <em>value object</em> is an instance of a non-abstract value class. All |
There was a problem hiding this comment.
Hello Alan, in several other places in the other JDK, we seem to refer to such classes as concrete value class. Should we do the same here instead of saying non-abstract value class?
There was a problem hiding this comment.
Defining a value object as an instance of non-abstract value classes works here, it's similar to the wording in the changes for the JLS.
There was a problem hiding this comment.
I would prefer to phrase it like "the class of a value object is a value class", more in line with code like o.getClass().isValue().
However, Valhalla models "identity" as the extra thing, so I guess it is better to go like "An identity object is an object whose class is an identity class or an array type. All other objects are value objects."
There was a problem hiding this comment.
I would prefer to keep this sentence as proposed as it keeps it consistent with the JLS and keeps the ordering that the terms are introduces consistent with the first sentence. I would be a bit wary of "All other objects are value objects" as it hints that there are other things that are also value classes.
| @@ -104,6 +105,7 @@ protected void finalize() { | |||
| * Test that the finalize method on an abstract value value is not invoked by the GC. | |||
There was a problem hiding this comment.
Typo - should have been "abstract value class ..."
jaikiran
left a comment
There was a problem hiding this comment.
Thank you for the update. This looks good to me.
liach
left a comment
There was a problem hiding this comment.
The introductory paragraph in class spec might be subject to further discussion to introduce people to "identity is the extra thing" view, but that shouldn't be a blocker.
Updates to java.lang.Object API docs:
A sanity test is added, this depends on the fix to finalizer registration in pull/2589.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/2588/head:pull/2588$ git checkout pull/2588Update a local copy of the PR:
$ git checkout pull/2588$ git pull https://git.openjdk.org/valhalla.git pull/2588/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 2588View PR using the GUI difftool:
$ git pr show -t 2588Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2588.diff
Using Webrev
Link to Webrev Comment