[mono] Reenable C4244 MSVC warning and fix occurrences #91395
Conversation
akoeplinger
force-pushed
the
fix-warning
branch
from
5f8052e to
4000cd6
Compare
akoeplinger
force-pushed
the
fix-warning
branch
from
4000cd6 to
41ecd2a
Compare
akoeplinger
requested review from
BrzVlad,
lambdageek,
fanyang-mono,
vargaz,
kotlarmilos,
SamMonoRT,
lateralusX,
thaystg and
marek-safar
as code owners
akoeplinger
force-pushed
the
fix-warning
branch
from
41ecd2a to
8350cc4
Compare
akoeplinger
force-pushed
the
fix-warning
branch
from
6408ddb to
1cc0da5
Compare
There was a problem hiding this comment.
Overall looks good, great work. Mainly a couple of additional places to use different conversion macros or opportunity to add a new more precise conversion macro.
The only thing to look out for is the mword SGenDescriptor related changes, mword is based on host size while SGenDescriptor is based on target size, so changing the type we will need to make sure that it doesn't introduce changed behavior or potentially loss of precision in case you have different host/target pointer sizes.
| @@ -14,7 +14,7 @@ | |||
| * "start" will point to the start of the next object, if the scanned | |||
| * object contained references. If not, the value of "start" should | |||
| * be considered undefined after executing this code. The object's | |||
| * GC descriptor must be in the variable "mword desc". | |||
| * GC descriptor must be in the variable "SgenDescriptor desc". | |||
There was a problem hiding this comment.
These are different type depending on host/target, mword is host machine word while SgenDescriptor is target machine word, have you validated that calls only uses SgenDescriptor and we don't end up casting mword to SgenDescriptor or SgenDescriptor to mword since that can lead to data loss depending on host/target combinations or depend on size of desc in logic etc.
There was a problem hiding this comment.
Yep I checked all the callsites and they are all called with SgenDescriptor type without casting.
ghost
locked as resolved and limited conversation to collaborators
Fixes #91249
We originally fixed the warnings with #69236 but due to
add_compile_options()calls in zlib.cmake all of the mono native libs - not just the zlib components - were being built with C4244 disabled and a few couple more leaked in.This fixes the occurrences and reworks the .cmake files a bit to hopefully prevent this in the future.