8373186: Improve readability of core reflection toString specifications #28688
Conversation
|
👋 Welcome back darcy! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
openjdk
bot
changed the title
openjdk
bot
added
csr
| * public static final int java.lang.Thread.MIN_PRIORITY | ||
| * private int java.io.FileDescriptor.fd | ||
| * public static java.util.List Foo.bar | ||
| * </pre> |
There was a problem hiding this comment.
Having such examples here means this method has a specified output that users can parse. We should move this to some non-normative section like an implementation note.
(Don't know why github mobile didn't capture my comment before, I already commented yesterday)
There was a problem hiding this comment.
Hmm. it is possible to move the examples so they are more clearly informative rather than normative.
As a counterpoint, the toString() spec for java.lang.annotation.Annotation states:
Returns a string representation of this annotation. The details of the representation are implementation-dependent, but the following may be regarded as typical:
@com.example.Name(first="Duke", middle="of", last="Java")
Developers used this string successfully for informative purposes, even though the exact details have been changed (improved and corrected) a number of times over the years.
| * by the field type, followed by a space, followed by | ||
| * the fully-qualified name of the class declaring the field, | ||
| * followed by a period, followed by the name of the field. | ||
| * {@return a string describing this {@code Field}} |
There was a problem hiding this comment.
Field::toString has been specified this way since JDK 1.1 and we have to assume there are frameworks or libraries that would break if we were ever change the format of the String representation. Would it be possible to put more context for the proposal in the JBS or PR? I think I'm mostly trying to understand if this is just about readability or whether an incompatible change to the string representation might follow.
There was a problem hiding this comment.
I don't have any implementation changes planned, but if there are new kinds of things added, I prefer to avoid treating the toString() method as a home for a context free grammar to provide all the necessary information about the item.
| * by the field type, followed by a space, followed by | ||
| * the fully-qualified name of the class declaring the field, | ||
| * followed by a period, followed by the name of the field. | ||
| * {@return a string describing this {@code Field}} |
There was a problem hiding this comment.
I don't see a compelling reason to backtrack on the format of toString.
Its likely to break some application or description of the field to a user.
There was a problem hiding this comment.
I think the reason to change the format is some future modifiers, like ACC_STRICT_INIT, won't get printed.
There was a problem hiding this comment.
Also, it helps avoid "attractive nuisance" questions like "how does the toString() of a bridge method indicate it is a bridge method"? (There is no such indication in the toString output, other than indirectly via the return type, but there is a Method.isBridge() predicate.)
| * @apiNote | ||
| * Specific information about {@linkplain #getModifiers() | ||
| * modifiers} or other aspects of the field should be retrieved | ||
| * using methods for that purpose. |
There was a problem hiding this comment.
This is needlessly vague and gives an opportunity to reinforce the preferred order of modifiers.
4 participants
Partial implementation of a spec refactoring to get some feedback before writing similar changes for other core reflection types.
Progress
Issues
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28688/head:pull/28688$ git checkout pull/28688Update a local copy of the PR:
$ git checkout pull/28688$ git pull https://git.openjdk.org/jdk.git pull/28688/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28688View PR using the GUI difftool:
$ git pr show -t 28688Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28688.diff
Using Webrev
Link to Webrev Comment