Skip to content

Fix warnings around mixing int and size_t usage in unsafe ways#34889

Closed
acoates-ms wants to merge 1 commit into
facebook:mainfrom
acoates-ms:layoutablewarning
Closed

Fix warnings around mixing int and size_t usage in unsafe ways#34889
acoates-ms wants to merge 1 commit into
facebook:mainfrom
acoates-ms:layoutablewarning

Conversation

@acoates-ms
Copy link
Copy Markdown
Contributor

Summary

react-native-windows builds treat a lot of c++ compiler warnings as errors. Including ones that cause value truncation when assigning size_t values to an int.

This fixes the compiler warning, by using size_t as the type for i, which matches size. I then modified the loop to avoid the value underflow that occurs when decrementing an unsigned zero value.

Changelog

[Internal] [Changed] - Fix compiler warnings around mixing int and size_t usage in unsafe ways

Test Plan

This code change was made in react-native-windows when integrating latest react-native changes. -- our fabric implementation was able to run through this code without any issues.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 6, 2022
@acoates-ms
Copy link
Copy Markdown
Contributor Author

This PR merged in this fix into react-native-windows: microsoft/react-native-windows#10688

@analysis-bot
Copy link
Copy Markdown

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,741,366 +54
android hermes armeabi-v7a 7,143,654 -9
android hermes x86 8,051,895 -85
android hermes x86_64 8,023,615 +67
android jsc arm64-v8a 9,608,659 +67
android jsc armeabi-v7a 8,373,912 -1
android jsc x86 9,555,978 -89
android jsc x86_64 10,148,477 +64

Base commit: abb21dd
Branch: main

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@rozele has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@rozele rozele linked an issue Oct 6, 2022 that may be closed by this pull request
@analysis-bot
Copy link
Copy Markdown

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: abb21dd
Branch: main

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @acoates-ms in 138c88c.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 6, 2022
@NickGerleman
Copy link
Copy Markdown
Contributor

I think you could pattern match off of the -Wpedantic change to add -Wextra and prevent as much error level mismatch in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Out of bounds access in LayoutableShadowNode

5 participants