Skip to content

[ADT] DenseMapInfo<unsigned long>::getHashValue - avoid MSVC out of bounds shift warning #96173

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

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

RKSimon
Copy link
Collaborator

@RKSimon RKSimon commented Jun 20, 2024

Fixes MSVC warning after #95734 - despite it taking the sizeof(Val) == 4 path, it still warns that the 32-bit unsigned long shift by 32 is out of bounds.

…ounds shift warning

Fixes MSVC warning after llvm#95734 - despite it taking the `sizeof(Val) == 4` path, it still warns that the 32-bit unsigned long shift by 32 is out of bounds, so avoid it by converting the hard coded shift amount to be based off sizeof() instead.
@llvmbot
Copy link
Member

llvmbot commented Jun 20, 2024

@llvm/pr-subscribers-llvm-adt

Author: Simon Pilgrim (RKSimon)

Changes

Fixes MSVC warning after #95734 - despite it taking the sizeof(Val) == 4 path, it still warns that the 32-bit unsigned long shift by 32 is out of bounds, so avoid it by converting the hard coded shift amount to be based off sizeof() instead.


Full diff: https://github.com/llvm/llvm-project/pull/96173.diff

1 Files Affected:

  • (modified) llvm/include/llvm/ADT/DenseMapInfo.h (+1-1)
diff --git a/llvm/include/llvm/ADT/DenseMapInfo.h b/llvm/include/llvm/ADT/DenseMapInfo.h
index 4f0aaa1ad48bb..3d83a2c1473f5 100644
--- a/llvm/include/llvm/ADT/DenseMapInfo.h
+++ b/llvm/include/llvm/ADT/DenseMapInfo.h
@@ -142,7 +142,7 @@ template<> struct DenseMapInfo<unsigned long> {
     if constexpr (sizeof(Val) == 4)
       return DenseMapInfo<unsigned>::getHashValue(Val);
     else
-      return detail::combineHashValue(Val >> 32, Val);
+      return detail::combineHashValue(Val >> (4 * sizeof(Val)), Val);
   }
 
   static bool isEqual(const unsigned long& LHS, const unsigned long& RHS) {

Copy link
Member

@ChuanqiXu9 ChuanqiXu9 left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM

@RKSimon RKSimon merged commit af82e63 into llvm:main Jun 20, 2024
4 of 6 checks passed
@RKSimon RKSimon deleted the msvc-densemapinfo-fix branch June 20, 2024 12:25
@RKSimon
Copy link
Collaborator Author

RKSimon commented Jun 20, 2024

Thank you everyone

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024
…ounds shift warning (llvm#96173)

Fixes MSVC warning after llvm#95734 - despite it taking the `sizeof(Val) == 4` path, it still warns that the 32-bit unsigned long shift by 32 is out of bounds.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants