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

8254271: Development to deprecate wrapper class constructors for removal #221

Closed

Conversation

RogerRiggs
Copy link
Collaborator

@RogerRiggs RogerRiggs commented Oct 9, 2020

The public constructors of the following classes have been deprecated since SE 9, and should be deprecated for removal in anticipation of these classes becoming inline classes.

java.lang.Boolean, Byte, Short, Integer, Long, Float, Double, and Character.

Existing uses of the constructors are converted to use the .valueOf(xxx) methods in classes and tests.


Progress

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

Testing

Linux x64 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) (1/9 failed) (1/9 failed) (1/9 failed)

Failed test tasks

Issue

  • JDK-8254271: Development to deprecate wrapper class constructors for removal

Reviewers

Download

$ git fetch https://git.openjdk.java.net/valhalla pull/221/head:pull/221
$ git checkout pull/221

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 9, 2020

👋 Welcome back rriggs! A progress list of the required criteria for merging this PR into jep390 will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Oct 9, 2020

@RogerRiggs this pull request will not be integrated until the CSR request JDK-8254324 for issue JDK-8252180 has been approved.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 9, 2020

Webrevs

@dansmithcode
Copy link
Collaborator

@dansmithcode dansmithcode commented Oct 9, 2020

Should change issue number to 8254271, which is targeted to 'repo-valhalla'. (I'm actually not sure what happens if you push with 8252180, which is targeted to 'tbd'...)

@RogerRiggs
Copy link
Collaborator Author

@RogerRiggs RogerRiggs commented Oct 9, 2020

Should change issue number to 8254271, which is targeted to 'repo-valhalla'. (I'm actually not sure what happens if you push with 8252180, which is targeted to 'tbd'...)

The fields in the bug are just hints for people. The actual push using git is specific to the repo and branch its targeted to.
The bugs are updated after the fact by 'hgupdater'.

@@ -773,7 +773,7 @@ public MemberName getDefinition() {
}

@Override
@SuppressWarnings("deprecation")
@SuppressWarnings({"deprecation", "removal"})
Copy link
Collaborator

@dansmithcode dansmithcode Oct 9, 2020

Choose a reason for hiding this comment

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

What's going to happen to these warning suppressions when we actually remove (make private) the constructors? Is the intent to just defer that problem until it happens?

Copy link
Collaborator Author

@RogerRiggs RogerRiggs Oct 9, 2020

Choose a reason for hiding this comment

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

When the code is changed to not use the constructors, the warning (deprecation and remove) should be removed.
Some of the code is upstream and requires more changes.

Copy link
Member

@mlchung mlchung Oct 10, 2020

Choose a reason for hiding this comment

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

We should change MemberName to call the factory method Byte::valueof (that's JDK code).

For the Graal change, you are touching the code anyway -- I think changing them to use the factory methods would be better than adding "removal" to the suppression. But it may be better to check if these tests intentionally test with the public constructors to test for JIT optimization before applying the change.

Copy link
Collaborator

@dansmithcode dansmithcode Oct 10, 2020

Choose a reason for hiding this comment

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

Yeah, Mandy replied to the question I should have asked: for all the suppressed "removal" points, what is the plan to fix the offending code? If not now, when? ("Later" may be a reasonable answer, but fleshing out the followup tasks would be helpful.)

Copy link
Collaborator Author

@RogerRiggs RogerRiggs Oct 13, 2020

Choose a reason for hiding this comment

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

The comment in MemberName makes it clear that hashCode method may be called before the cache is setup and valueOf uses the cache.

Copy link
Collaborator Author

@RogerRiggs RogerRiggs Oct 13, 2020

Choose a reason for hiding this comment

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

Its up to the Graal folks to replace their code. In some cases, they are depending on Identity semantics of new Integer().

Copy link
Member

@mlchung mlchung Oct 13, 2020

Choose a reason for hiding this comment

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

Thanks for the clarification. Can you file a JBS issue to Graal to make change in its upstream project?

@dansmithcode
Copy link
Collaborator

@dansmithcode dansmithcode commented Oct 9, 2020

Should change issue number to 8254271, which is targeted to 'repo-valhalla'. (I'm actually not sure what happens if you push with 8252180, which is targeted to 'tbd'...)

The fields in the bug are just hints for people. The actual push using git is specific to the repo and branch its targeted to.
The bugs are updated after the fact by 'hgupdater'.

Understood.

I'm saying it needs to be configured so that 'hgupdater' leaves 8252180 open and closes 8254271. (And I'm unsure of exactly how things will go wrong otherwise. I know sometimes hgupdater will create backport issues...)

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 9, 2020

Mailing list message from Roger Riggs on valhalla-dev:

Hg updater updates whatever issue/task id is used in the commit.
Having two Jira issues where only one is needed is a bad idea.

Roger

On 10/9/20 5:42 PM, Dan Smith wrote:

@@ -0,0 +1,62 @@
/*
Copy link
Member

@jddarcy jddarcy Oct 9, 2020

Choose a reason for hiding this comment

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

We have historically not found it necessary to write tests to verify this kind of change and have instead relied on signature testing.

Copy link
Member

@mlchung mlchung Oct 10, 2020

Choose a reason for hiding this comment

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

+1.

FWIW. You have also verified when you get the warnings from JDK build and needs to suppressed it.

Copy link
Collaborator Author

@RogerRiggs RogerRiggs Oct 13, 2020

Choose a reason for hiding this comment

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

The test can be removed but was a double check that all of the constructors of the named classes had been modified.

@@ -438,8 +438,8 @@ private static void initialize()
private static void defineEntity( String name, char value )
{
if ( _byName.get( name ) == null ) {
_byName.put( name, new Integer( value ) );
Copy link
Member

@jddarcy jddarcy Oct 9, 2020

Choose a reason for hiding this comment

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

Please make sure @JoeWang-Java reviews the XML changes; there are complications in some areas where the master code is upstream of the code in the JDK.

Copy link
Member

@mlchung mlchung Oct 10, 2020

Choose a reason for hiding this comment

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

JAXP was subsumed by JSR 379 in Java SE 9.

@dansmithcode
Copy link
Collaborator

@dansmithcode dansmithcode commented Oct 13, 2020

Clarification on integration behavior:

The model is that each pull to valhalla or jdk needs a separate bugid. That's the workflow we need to follow unless we can flesh out and get tooling support for an alternative model.

I'm asking that 8252180 be reserved for the jdk pull and for the CSR. And that all pulls to valhalla use bugids of subtasks of that issue, such as 8254271.

Robin says if you push with bugid 8252180 (which is targeted to 'tbd'), the issue will be closed, disrupting its use for the jdk pull and CSR. He will consider adding a guard that warns about the mismatch of JBS target and PR target to prevent this sort of accident.

@RogerRiggs RogerRiggs changed the title 8252180: [JEP 390] Deprecate wrapper class constructors for removal Development to deprecate wrapper class constructors for removal Oct 13, 2020
@RogerRiggs
Copy link
Collaborator Author

@RogerRiggs RogerRiggs commented Oct 13, 2020

The CSR for this change will be made against 8252180

@openjdk openjdk bot removed the rfr label Oct 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

@RogerRiggs usage: /csr [needed|unneeded], requires that the issue the pull request refers to links to an approved CSR request.

@RogerRiggs RogerRiggs changed the title Development to deprecate wrapper class constructors for removal 8254271: Development to deprecate wrapper class constructors for removal Oct 13, 2020
@openjdk openjdk bot added the rfr label Oct 13, 2020
@@ -773,7 +773,7 @@ public MemberName getDefinition() {
}

@Override
@SuppressWarnings("deprecation")
@SuppressWarnings({"deprecation", "removal"})
Copy link
Member

@mlchung mlchung Oct 13, 2020

Choose a reason for hiding this comment

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

Thanks for the clarification. Can you file a JBS issue to Graal to make change in its upstream project?

@RogerRiggs
Copy link
Collaborator Author

@RogerRiggs RogerRiggs commented Oct 13, 2020

/csr unneeded

@openjdk openjdk bot removed the csr label Oct 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

@RogerRiggs determined that a CSR request is not needed for this pull request.

@RogerRiggs
Copy link
Collaborator Author

@RogerRiggs RogerRiggs commented Oct 13, 2020

Please review the CSR for 8252180, to be used for the integration

CSR 8254324: [JEP 390] Deprecate wrapper class constructors for removal

@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2020

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

8254271: Development to deprecate wrapper class constructors for removal

Reviewed-by: mchung

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 no new commits pushed to the jep390 branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the jep390 branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Oct 13, 2020
@dansmithcode
Copy link
Collaborator

@dansmithcode dansmithcode commented Oct 13, 2020

Have you considered rephrasing the reference to constructors in the valueOf methods?:

     * If a new {@code Byte} instance is not required, this method
     * should generally be used in preference to the constructor
     * {@link #Byte(byte)}

The message now is "Don't use the constructors! Don't try to get a new instance!"

@RogerRiggs
Copy link
Collaborator Author

@RogerRiggs RogerRiggs commented Nov 2, 2020

Have you considered rephrasing the reference to constructors in the valueOf methods?:

     * If a new {@code Byte} instance is not required, this method
     * should generally be used in preference to the constructor
     * {@link #Byte(byte)}

The message now is "Don't use the constructors! Don't try to get a new instance!"

I think "if ... is not required" covers the case already. The behavior of Integer is not changing.
The valueOf() method is already the recommendation.

@RogerRiggs
Copy link
Collaborator Author

@RogerRiggs RogerRiggs commented Nov 6, 2020

/integrate

@openjdk openjdk bot closed this Nov 6, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 6, 2020

@RogerRiggs Since your change was applied there have been 2 commits pushed to the jep390 branch:

  • 2ac97b2: 8254274: Development to add 'lint' warning for @valuebased classes
  • f2f03c8: 8255125: [Development] copy jdk.internal.ValueBased to jep390 branch

Your commit was automatically rebased without conflicts.

Pushed as commit 0c2ef39.

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

@RogerRiggs RogerRiggs deleted the wrapper-deprecate-8252180 branch Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants