-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix type conversion error in BitFlags Clear #30680
Fix type conversion error in BitFlags Clear #30680
Conversation
PR #30680: Size comparison from 8f76c06 to 0f203cc Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #30680: Size comparison from fda14c6 to f6b15a7 Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
PR #30680: Size comparison from fda14c6 to d0ff8e2 Increases above 0.2%:
Increases (19 builds for cc13x4_26x4, cyw30739, efr32, linux, mbed, psoc6, qpg, telink)
Decreases (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
@andy31415 I've added the test here. |
@@ -102,7 +102,7 @@ class BitFlags | |||
*/ | |||
BitFlags & Clear(const BitFlags & other) | |||
{ | |||
mValue &= ~other.mValue; | |||
mValue &= static_cast<IntegerType>(~static_cast<IntegerType>(other.mValue)); |
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.
This is probably not right. The right thing is:
mValue = static_cast<IntegerType>(mValue & ~other.mValue);
or so. Because using &=
causes the result of the &
(which may have been promoted to a different type) to be assigned to mValue...
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.
could you provide a test showing this behaviour?
We relied a lot on unit tests to prove that bitflags works for us as expected.
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.
It works correctly. When it compiles. It fails to compile on some compilers, I expect. Depending on optimization settings and the like. Because it triggers -Wconversion
warnings due to assigning the int
result of &
into whatever type mValue
is.
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.
Can we have a compile variant test for this? When this was fixed, we added a unit test that validated we compile.
fixes #30679