8379337: Deprecate Modifier.toString#30093
Conversation
…ecate-modifier-tostring
|
👋 Welcome back liach! A progress list of the required criteria for merging this PR into |
|
@liach 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 12 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. ➡️ To integrate this PR with the above commit message to the |
|
@liach The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| * have a corresponding access flag</li> | ||
| * <li>The same bit may represent different flags in different structures, like | ||
| * {@code 0x0040} represents {@link #VOLATILE ACC_VOLATILE} in fields and | ||
| * {@code ACC_BRIDGE} in interfaces</li> |
There was a problem hiding this comment.
ACC_BRIDGE applies to all bridge methods, not only those on interfaces.
| * {@code ACC_BRIDGE} in interfaces</li> | |
| * {@code ACC_BRIDGE} in methods</li> |
Marcono1234
left a comment
There was a problem hiding this comment.
Hopefully these review comments are useful. Feel free to only consider them as suggestions.
| * </ol> | ||
| * These assumptions are no longer valid: | ||
| * <ol> | ||
| * <li>Java language modifiers like method modifier {@code default} does not |
There was a problem hiding this comment.
Maybe grammar issue: This refers to the plural "modifiers"
| * <li>Java language modifiers like method modifier {@code default} does not | |
| * <li>Java language modifiers like method modifier {@code default} do not |
| * {@code 0x0040} represents {@link #VOLATILE ACC_VOLATILE} in fields and | ||
| * {@code ACC_BRIDGE} in interfaces</li> | ||
| * </ol> | ||
| * To mitigate, {@code Modifier} class only include source modifiers that |
There was a problem hiding this comment.
Maybe grammar issue
| * To mitigate, {@code Modifier} class only include source modifiers that | |
| * To mitigate, the {@code Modifier} class only includes source modifiers that |
| * objects, such as {@link Class#accessFlags()}. | ||
| * <p> | ||
| * To print an access flags value for debug output, consider using the | ||
| * format {@code %04x} instead of this method; this method omits all class |
There was a problem hiding this comment.
Maybe this should say "hex format" or similar to make it immediately obvious what this pattern does?
| * format {@code %04x} instead of this method; this method omits all class | |
| * hex format {@code %04x} instead of this method; this method omits all class |
There was a problem hiding this comment.
I used 'format' because you can use this out of the box in String.format, printf, and String.formatted. 'hex format' I'm afraid would make users think they need to use java.util.HexFormat, I haven't tried using that to print yet.
| * modifiers that can be applied to a class. | ||
| * | ||
| * @deprecated | ||
| * This method is intended to be used in conjunction with {@link #toString() |
There was a problem hiding this comment.
Link to wrong toString method? toString() is not deprecated only toString(int)?
| * This method is intended to be used in conjunction with {@link #toString() | |
| * This method is intended to be used in conjunction with {@link #toString(int) |
(same also below for all other occurrences)
| * interface (JLS {@jls 9.1.1}). | ||
| * | ||
| * @deprecated | ||
| * This method tries to describe the source modifiers from an access flags |
There was a problem hiding this comment.
I agree that the methods should be deprecated but I think the @deprecated text will need a few iterations. I think it would be better to start with something like "This method was originally specified to describe the source modifiers ..." rathe rthan "This tries ...".
| * {@jls 9.4.3}) and the {@code sealed} and {@code non-sealed} class | ||
| * (JLS {@jls 8.1.1.2}) and interface (JLS {@jls 9.1.1.4}) modifiers | ||
| * are <em>not</em> represented in this class. | ||
| * {@code Modifier} was designed with two assumptions: |
There was a problem hiding this comment.
I think the apiNote will need a few iterations on the wording. I don't like it starting with the assumptions and then saying that the assumptions aren't valid. Instead I think it would be better to say that the translation of source modifiers to access flags has evolved since the API was originally introduced. In addition, the bit may represent different flags. The API can still be used for source modifiers that directly map to an access flag. New code is encouraged to use AccessFlags.
…ecate-modifier-tostring
|
@liach This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
The total number of required reviews for this PR has been set to 2 based on the presence of this label: |
|
/template append |
|
@liach The pull request template has been appended to the pull request body |
|
@liach |
…ecate-modifier-tostring
| * <p> | ||
| * Java SE 5.0 introduced many new access and property flags, such as {@link | ||
| * AccessFlag#SYNTHETIC ACC_SYNTHETIC}. Unlike {@code ACC_SUPER}, these access | ||
| * and property flags are reported by the {@code getModifier()} methods. |
There was a problem hiding this comment.
Should this be getModifiers()?
| * and property flags are reported by the {@code getModifier()} methods. | |
| * and property flags are reported by the {@code getModifiers()} methods. |
…ecate-modifier-tostring
|
@liach |
|
/label remove hotspot-runtime |
|
@liach |
…ecate-modifier-tostring
| * <p> | ||
| * Modifier interpretation is context-sensitive: for example, the bit checked by | ||
| * {@link #isSynchronized(int) isSynchronized} only represents the {@code | ||
| * synchronized} modifier on methods, so a {@code true} return on a field |
There was a problem hiding this comment.
The first paragraph uses "flags", the second paragraph switches to "bits", a term that is never used again. The referenced AccessFlags or javax.lang.model.element.Modifier doesn't speak of bits. So I'm wondering if it would be better to say "the modifier checked by ..." rather than "bit checked".
There was a problem hiding this comment.
I will use "access flags" consistently.
| * | ||
| * @deprecated | ||
| * This method exists solely to support the now-deprecated | ||
| * {@link #toString(int) Modifier::toString(int)} method. |
There was a problem hiding this comment.
What would think of changing the first sentence to "This method was originally created to the support ..". The second sentence is good, sends the reader to the new API.
AlanBateman
left a comment
There was a problem hiding this comment.
I think this is mostly looks good, just some minor word smithing on the docs change.
…ecate-modifier-tostring
|
/integrate Thanks for the reviews! |
|
Going to push as commit cb8860a.
Your commit was automatically rebased without conflicts. |
In project valhalla development, we discovered that
Modifier.toStringbecomes more problematic than helpful: there's now a mix-and-match of modifiers from access flags and other class file sources, for example, classes now can have ACC_IDENTITY, which clashes with ACC_SYNCHRONIZED, and the correct modifier to reflect, "value", must be deduced by the users manually.With fewer bits available for the access flags, it becomes more apparent that future source modifiers will no longer be reflected by
Modifier.toString, and future access flags are more likely to be reflected incorrectly. Thus, we should dissuade users from this API in the long run.Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/30093/head:pull/30093$ git checkout pull/30093Update a local copy of the PR:
$ git checkout pull/30093$ git pull https://git.openjdk.org/jdk.git pull/30093/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 30093View PR using the GUI difftool:
$ git pr show -t 30093Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/30093.diff
Using Webrev
Link to Webrev Comment