-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Several compilation warnings resolved #1106
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
Conversation
IndexPath(indexes: [Int.max >> 8, 2, Int.max >> 36]).hashValue // this should not cause an overflow crash | ||
|
||
// this should not cause an overflow crash | ||
let hash: Int? = IndexPath(indexes: [Int.max >> 8, 2, Int.max >> 36]).hashValue |
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.
Is this reasonable? Or is guaranteed that compiler will not optimize out the below to a no-op?
_ = IndexPath(indexes: [Int.max >> 8, 2, Int.max >> 36]).hashValue
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.
eh the assert below validates the non-crash case; and it is a test of an overflow so I think it should be fine as you have it
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 good to me, I have the same length removal in another pr but I will deal with that conflict when it comes to be merged
IndexPath(indexes: [Int.max >> 8, 2, Int.max >> 36]).hashValue // this should not cause an overflow crash | ||
|
||
// this should not cause an overflow crash | ||
let hash: Int? = IndexPath(indexes: [Int.max >> 8, 2, Int.max >> 36]).hashValue |
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.
eh the assert below validates the non-crash case; and it is a test of an overflow so I think it should be fine as you have it
@swift-ci please test and merge |
This was removed in `swift-corelibs-foundation` in swiftlang/swift-corelibs-foundation#1106 Make the same change to the overlay version, to keep them in sync.
Several compilation warnings resolved