-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8256633: Fix product build on Windows+Arm64 #1312
Conversation
Fix this warning: ``` C:\work\openjdk-jdk\src\hotspot\cpu\aarch64\assembler_aarch64.hpp(508): error C2220: the following warning is treated as an error C:\work\openjdk-jdk\src\hotspot\cpu\aarch64\assembler_aarch64.hpp(508): warning C4390: ';': empty controlled statement found; is this the intent? ```
👋 Welcome back burban! A progress list of the required criteria for merging this PR into |
Looks good and trivial. (And a good reminder why if statements without curly braces seldom are a good thing...) |
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 fine. Not sure how sensible are negative shifts, but new code follows the current semantics exactly.
@lewurm 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 52 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 (@shipilev) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
/integrate |
/sponsor |
@shipilev @lewurm Since your change was applied there have been 52 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit f576628. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Thank you Aleksey and Magnus. |
assert(_ext.shift() == (int)size, "bad shift"); | ||
} | ||
i->f(_ext.shift() > 0, 12); |
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.
IMO, if this is to be fixed it should be fixed properly, viz:
--- a/src/hotspot/share/utilities/debug.hpp
+++ b/src/hotspot/share/utilities/debug.hpp
@@ -45,7 +45,7 @@ bool handle_assert_poison_fault(const void* ucVoid, const void* faulting_address
// assertions
#ifndef ASSERT
-#define vmassert(p, ...)
+#define vmassert(p, ...) do { } while (0)
#else
Fix this warning:
Thanks to @magicus to bring that to my attention.
Progress
Testing
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/1312/head:pull/1312
$ git checkout pull/1312