-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
8310332: Fix -Wconversion warnings in MethodData #14557
Conversation
👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable. Bare narrowing casts, though, give me hives: they're an accident waiting to happen. Having said that, these look safe enough.
@coleenp 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 no new commits pushed to the ➡️ To integrate this PR with the above commit message to the |
Thank you Andrew. I tried to only use narrowing casts where they're obviously correct because checked_cast<> comes with its own set of problems (and my builds seem to be getting slower). Thanks for reviewing. |
Mailing list message from Andrew Haley on hotspot-dev: On 6/20/23 14:23, Coleen Phillimore wrote:
Do tell. :-)
-- |
Mailing list message from Andrew Haley on hotspot-dev: On 6/20/23 14:59, Andrew Haley wrote:
Never mind, I just looked at the palaver with checked_cast in PR 8309685. -- |
Mailing list message from coleen.phillimore at oracle.com on hotspot-dev: On 6/21/23 8:48 AM, Andrew Haley wrote:
palaver? Yes, checked_cast<> was super unhappy with sign extensions, |
Thank you Andrew and Fred for reviewing. |
Going to push as commit fd1163d.
Your commit was automatically rebased without conflicts. |
Please review this change to narrow parameter and return types to avoid implicit casts, add casts where obvious, add checked_cast<> where not obvious and changed the size of a couple parameters to not cast to the smaller size.
I didn't change TypeProfileWidth and BciProfileWidth because jvmci and SA care about the sizes.
Tested tier1 all Oracle supported platforms, and tier1-4 linux + windows x64.
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14557/head:pull/14557
$ git checkout pull/14557
Update a local copy of the PR:
$ git checkout pull/14557
$ git pull https://git.openjdk.org/jdk.git pull/14557/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 14557
View PR using the GUI difftool:
$ git pr show -t 14557
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14557.diff
Webrev
Link to Webrev Comment