Add explicit type casts to fix compilation errors on Windows. #31363
Conversation
facebook-github-bot
added
the
CLA Signed
rectified95
added
p: Microsoft
Base commit: 2b49664 |
Base commit: 9e020ef |
|
It would be good to add some CI that this type of stuff isn't regressed since it seems fragile and breaks us every time someone forgets to use the right cast (or otherwise deal with the overflow) :) |
|
Maybe we can enable equivalent Clang warnings when building with BUCK? I.e. enable the same warnings for internal usage on iOS/Android. This sort of thing is going to happen if warnings are mismatched between repos. If we've decided we care about SDL warnings, I think our team should look at what is needed to keep similar warnings clean upstream. |
|
I do think this is something our team has some responsibility for. We have stricter warnings, and realized there are warnings we no longer want to disable in upstream code. It's great we were able to get things clean, but keeping things clean long term is always the hard part that needs enforcement. |
|
Something that I think we could use help with is upstreaming any additional fixes. E.g. Yoga no longer responds to external PRs, so there have been fixes we haven't been able to do anything with there. |
|
@rozele has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
rectified95
force-pushed
the
mapbuffer_windows
branch
from
675099a to
3c880a8
Compare
rectified95
force-pushed
the
mapbuffer_windows
branch
from
3c880a8 to
d6ed4c6
Compare
| int32_t bufferSize = keyValuesOffset_ + dynamicDataOffset_; | ||
|
|
||
| _header.bufferSize = bufferSize; | ||
| _header.bufferSize = static_cast<uint16_t>(bufferSize); |
There was a problem hiding this comment.
I came across this and I'm not certain truncating to 16 bit is correct behavior. There was a recent change to move more of this class to use int32_t though.
There was a problem hiding this comment.
I'm going to test removing this + the case below locally in RNW. Though the above still makes sense (since string length is size_t).
There was a problem hiding this comment.
This indeed now builds without these 2 casts - removing.
Still, this gets implicitly cast to a narrower data type - OOC why do you think explicit casting is wrong here?
rectified95
force-pushed
the
mapbuffer_windows
branch
from
d6ed4c6 to
f809db0
Compare
|
@rozele I think this is in a state it should be now. |
|
@rozele has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
5 participants
Summary
Recent changes to
MapBufferhave broken the compilation on Windows.This fix is similar to this recently-merged change: #31106
Side note - this PR only addresses a build break, but doesn't address the unsafe casting semantics in
MapBuffer, which can still cause overflows.Changelog
[General] [Fixed] - Fix compilation errors on Windows.
Test Plan
RN now builds in Visual Studio on Windows.