Enable -Wconversion (facebook#1359)

Summary: X-link: facebook/react-native#39291 This enables clang warnings around potentially unsafe conversions, such as those with mismatched signedness, or ones which may lead to truncation. This should catch issues in local development which create errors for MSVC (e.g. Dash), who's default `/W3` includes warnings akin to `-Wshorten-64-to-32`. This full set of warnings here is a tad spammy, but probably more useful than not. Changelog: [Internal] Reviewed By: yungsters Differential Revision: D48954777
NickGerleman · Sep 5, 2023 · 52838c8 · 52838c8
1 parent 95a7b44
commit 52838c8
Show file tree

Hide file tree

Showing 17 changed files with 147 additions and 123 deletions.
diff --git a/Yoga.podspec b/Yoga.podspec
@@ -32,6 +32,7 @@ Pod::Spec.new do |spec|
       '-Wall',
       '-Werror',
       '-Wextra',
+      '-Wconversion',
       '-std=c++17',
       '-fPIC'
   ]

diff --git a/benchmark/YGBenchmark.c b/benchmark/YGBenchmark.c
@@ -55,7 +55,8 @@ static void __printBenchmarkResult(
   double mean = 0;
   clock_t lastEnd = start;
   for (uint32_t i = 0; i < NUM_REPETITIONS; i++) {
-    timesInMs[i] = (endTimes[i] - lastEnd) / (double) CLOCKS_PER_SEC * 1000;
+    timesInMs[i] =
+        ((double) (endTimes[i] - lastEnd)) / (double) CLOCKS_PER_SEC * 1000;
     lastEnd = endTimes[i];
     mean += timesInMs[i];
   }

diff --git a/cmake/project-defaults.cmake b/cmake/project-defaults.cmake
@@ -35,6 +35,7 @@ add_compile_options(
     -Wall
     -Wextra
     -Werror
+    -Wconversion
     # Disable RTTI
     $<$<COMPILE_LANGUAGE:CXX>:-fno-rtti>
     # Use -O2 (prioritize speed)

diff --git a/java/jni/YGJNI.h b/java/jni/YGJNI.h
@@ -22,12 +22,12 @@ namespace {
 const int HAS_NEW_LAYOUT = 16;
 
 union YGNodeContext {
-  uintptr_t edgesSet = 0;
+  int32_t edgesSet = 0;
   void* asVoidPtr;
 };
 
 class YGNodeEdges {
-  uintptr_t edges_;
+  int32_t edges_;
 
 public:
   enum Edge {

diff --git a/java/jni/YGJNIVanilla.cpp b/java/jni/YGJNIVanilla.cpp
@@ -146,7 +146,7 @@ static int YGJNILogFunc(
   va_list argsCopy;
   va_copy(argsCopy, args);
   int result = vsnprintf(nullptr, 0, format, argsCopy);
-  std::vector<char> buffer(1 + result);
+  std::vector<char> buffer(1 + static_cast<size_t>(result));
   vsnprintf(buffer.data(), buffer.size(), format, args);
 
   auto jloggerPtr =
@@ -236,7 +236,9 @@ static void jni_YGNodeInsertChildJNI(
     jlong childPointer,
     jint index) {
   YGNodeInsertChild(
-      _jlong2YGNodeRef(nativePointer), _jlong2YGNodeRef(childPointer), index);
+      _jlong2YGNodeRef(nativePointer),
+      _jlong2YGNodeRef(childPointer),
+      static_cast<uint32_t>(index));
 }
 
 static void jni_YGNodeSwapChildJNI(
@@ -246,7 +248,9 @@ static void jni_YGNodeSwapChildJNI(
     jlong childPointer,
     jint index) {
   YGNodeSwapChild(
-      _jlong2YGNodeRef(nativePointer), _jlong2YGNodeRef(childPointer), index);
+      _jlong2YGNodeRef(nativePointer),
+      _jlong2YGNodeRef(childPointer),
+      static_cast<uint32_t>(index));
 }
 
 static void jni_YGNodeSetIsReferenceBaselineJNI(
@@ -307,13 +311,13 @@ static void YGTransferLayoutOutputsRecursive(
   const int arrSize = 6 + (marginFieldSet ? 4 : 0) + (paddingFieldSet ? 4 : 0) +
       (borderFieldSet ? 4 : 0);
   float arr[18];
-  arr[LAYOUT_EDGE_SET_FLAG_INDEX] = fieldFlags;
+  arr[LAYOUT_EDGE_SET_FLAG_INDEX] = static_cast<float>(fieldFlags);
   arr[LAYOUT_WIDTH_INDEX] = YGNodeLayoutGetWidth(root);
   arr[LAYOUT_HEIGHT_INDEX] = YGNodeLayoutGetHeight(root);
   arr[LAYOUT_LEFT_INDEX] = YGNodeLayoutGetLeft(root);
   arr[LAYOUT_TOP_INDEX] = YGNodeLayoutGetTop(root);
   arr[LAYOUT_DIRECTION_INDEX] =
-      static_cast<jint>(YGNodeLayoutGetDirection(root));
+      static_cast<float>(YGNodeLayoutGetDirection(root));
   if (marginFieldSet) {
     arr[LAYOUT_MARGIN_START_INDEX] = YGNodeLayoutGetMargin(root, YGEdgeLeft);
     arr[LAYOUT_MARGIN_START_INDEX + 1] = YGNodeLayoutGetMargin(root, YGEdgeTop);
@@ -670,9 +674,10 @@ static YGSize YGJNIMeasureFunc(
         sizeof(measureResult) == 8,
         "Expected measureResult to be 8 bytes, or two 32 bit ints");
 
-    int32_t wBits = 0xFFFFFFFF & (measureResult >> 32);
-    int32_t hBits = 0xFFFFFFFF & measureResult;
+    uint32_t wBits = 0xFFFFFFFF & (measureResult >> 32);
+    uint32_t hBits = 0xFFFFFFFF & measureResult;
 
+    // TODO: this is unsafe under strict aliasing and should use bit_cast
     const float* measuredWidth = reinterpret_cast<float*>(&wBits);
     const float* measuredHeight = reinterpret_cast<float*>(&hBits);
 

diff --git a/java/jni/YGJTypesVanilla.h b/java/jni/YGJTypesVanilla.h
@@ -14,7 +14,7 @@
 #include "jni.h"
 
 class PtrJNodeMapVanilla {
-  std::map<YGNodeConstRef, size_t> ptrsToIdxs_{};
+  std::map<YGNodeConstRef, jsize> ptrsToIdxs_{};
   jobjectArray javaNodes_{};
 
 public:
@@ -25,13 +25,13 @@ class PtrJNodeMapVanilla {
     using namespace facebook::yoga::vanillajni;
 
     JNIEnv* env = getCurrentEnv();
-    size_t nativePointersSize = env->GetArrayLength(javaNativePointers);
-    std::vector<jlong> nativePointers(nativePointersSize);
+    jsize nativePointersSize = env->GetArrayLength(javaNativePointers);
+    std::vector<jlong> nativePointers(static_cast<size_t>(nativePointersSize));
     env->GetLongArrayRegion(
         javaNativePointers, 0, nativePointersSize, nativePointers.data());
 
-    for (size_t i = 0; i < nativePointersSize; ++i) {
-      ptrsToIdxs_[(YGNodeConstRef) nativePointers[i]] = i;
+    for (jsize i = 0; i < nativePointersSize; ++i) {
+      ptrsToIdxs_[(YGNodeConstRef) nativePointers[static_cast<size_t>(i)]] = i;
     }
   }
 

diff --git a/java/jni/common.cpp b/java/jni/common.cpp
@@ -18,7 +18,8 @@ void registerNatives(
 
   assertNoPendingJniExceptionIf(env, !clazz);
 
-  auto result = env->RegisterNatives(clazz, methods, numMethods);
+  auto result =
+      env->RegisterNatives(clazz, methods, static_cast<int32_t>(numMethods));
 
   assertNoPendingJniExceptionIf(env, result != JNI_OK);
 }

diff --git a/tests/NumericBitfieldTest.cpp b/tests/NumericBitfieldTest.cpp
@@ -13,7 +13,7 @@
 namespace facebook::yoga {
 
 TEST(NumericBitfield, one_boolean_defaults_to_false) {
-  constexpr uint8_t flags = 0;
+  constexpr uint32_t flags = 0;
 
   ASSERT_EQ(getBooleanData(flags, 0), false);
   static_assert(
@@ -22,7 +22,7 @@ TEST(NumericBitfield, one_boolean_defaults_to_false) {
 }
 
 TEST(NumericBitfield, one_boolean_can_be_initialized_to_true) {
-  constexpr uint8_t flags = 1;
+  constexpr uint32_t flags = 1;
 
   ASSERT_EQ(getBooleanData(flags, 0), true);
   static_assert(
@@ -31,14 +31,14 @@ TEST(NumericBitfield, one_boolean_can_be_initialized_to_true) {
 }
 
 TEST(NumericBitfield, one_boolean_can_be_set_to_true) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
 
   setBooleanData(flags, 0, true);
   ASSERT_EQ(getBooleanData(flags, 0), true);
 }
 
 TEST(NumericBitfield, second_boolean_defaults_to_false) {
-  constexpr uint8_t flags = 0;
+  constexpr uint32_t flags = 0;
 
   ASSERT_EQ(getBooleanData(flags, 1), false);
   static_assert(
@@ -47,7 +47,7 @@ TEST(NumericBitfield, second_boolean_defaults_to_false) {
 }
 
 TEST(NumericBitfield, second_boolean_can_be_initialized_to_true) {
-  constexpr uint8_t flags = 2;
+  constexpr uint32_t flags = 2;
 
   ASSERT_EQ(getBooleanData(flags, 0), false);
   ASSERT_EQ(getBooleanData(flags, 1), true);
@@ -60,15 +60,15 @@ TEST(NumericBitfield, second_boolean_can_be_initialized_to_true) {
 }
 
 TEST(NumericBitfield, second_boolean_can_be_set_to_true) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
 
   setBooleanData(flags, 1, true);
   ASSERT_EQ(getBooleanData(flags, 0), false);
   ASSERT_EQ(getBooleanData(flags, 1), true);
 }
 
 TEST(NumericBitfield, third_boolean_defaults_to_false) {
-  constexpr uint8_t flags = 0;
+  constexpr uint32_t flags = 0;
 
   ASSERT_EQ(getBooleanData(flags, 2), false);
   static_assert(
@@ -77,7 +77,7 @@ TEST(NumericBitfield, third_boolean_defaults_to_false) {
 }
 
 TEST(NumericBitfield, third_boolean_can_be_initialized_to_true) {
-  constexpr uint8_t flags = 4;
+  constexpr uint32_t flags = 4;
 
   ASSERT_EQ(getBooleanData(flags, 0), false);
   ASSERT_EQ(getBooleanData(flags, 1), false);
@@ -94,7 +94,7 @@ TEST(NumericBitfield, third_boolean_can_be_initialized_to_true) {
 }
 
 TEST(NumericBitfield, third_boolean_can_be_set_to_true) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
 
   setBooleanData(flags, 2, true);
   ASSERT_EQ(getBooleanData(flags, 0), false);
@@ -103,7 +103,7 @@ TEST(NumericBitfield, third_boolean_can_be_set_to_true) {
 }
 
 TEST(NumericBitfield, setting_boolean_values_does_not_spill_over) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
 
   setBooleanData(flags, 1, (bool) 7);
 
@@ -113,7 +113,7 @@ TEST(NumericBitfield, setting_boolean_values_does_not_spill_over) {
 }
 
 TEST(NumericBitfield, first_enum_defaults_to_0) {
-  constexpr uint8_t flags = 0;
+  constexpr uint32_t flags = 0;
 
   ASSERT_EQ(getEnumData<YGAlign>(flags, 0), YGAlignAuto);
   static_assert(
@@ -122,15 +122,15 @@ TEST(NumericBitfield, first_enum_defaults_to_0) {
 }
 
 TEST(NumericBitfield, first_enum_can_be_set) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
 
   setEnumData<YGAlign>(flags, 0, YGAlignSpaceBetween);
 
   ASSERT_EQ(getEnumData<YGAlign>(flags, 0), YGAlignSpaceBetween);
 }
 
 TEST(NumericBitfield, second_enum_defaults_to_0) {
-  constexpr uint8_t flags = 0;
+  constexpr uint32_t flags = 0;
   static constexpr size_t alignOffset = 0;
   static constexpr size_t edgeOffset = 3;
 
@@ -145,7 +145,7 @@ TEST(NumericBitfield, second_enum_defaults_to_0) {
 }
 
 TEST(NumericBitfield, second_enum_can_be_set) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
   static constexpr size_t alignOffset = 0;
   static constexpr size_t edgeOffset = 3;
 
@@ -156,7 +156,7 @@ TEST(NumericBitfield, second_enum_can_be_set) {
 }
 
 TEST(NumericBitfield, third_enum_defaults_to_0) {
-  constexpr uint8_t flags = 0;
+  constexpr uint32_t flags = 0;
   static constexpr size_t alignOffset = 0;
   static constexpr size_t boolOffset = 3;
   static constexpr size_t edgesOffset = 4;
@@ -176,7 +176,7 @@ TEST(NumericBitfield, third_enum_defaults_to_0) {
 }
 
 TEST(NumericBitfield, third_enum_can_be_set) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
   static constexpr size_t alignOffset = 0;
   static constexpr size_t boolOffset = 3;
   static constexpr size_t edgesOffset = 4;
@@ -189,12 +189,13 @@ TEST(NumericBitfield, third_enum_can_be_set) {
 }
 
 TEST(NumericBitfield, setting_values_does_not_spill_over) {
-  uint8_t flags = 0;
+  uint32_t flags = 0;
   static constexpr size_t alignOffset = 0;
   static constexpr size_t edgesOffset = 3;
   static constexpr size_t boolOffset = 7;
 
-  setEnumData<YGEdge>(flags, edgesOffset, (YGEdge) 0xffffff);
+  uint32_t edge = 0xffffff;
+  setEnumData<YGEdge>(flags, edgesOffset, (YGEdge) edge);
 
   ASSERT_EQ(getEnumData<YGAlign>(flags, alignOffset), 0);
   ASSERT_EQ(getBooleanData(flags, boolOffset), false);

diff --git a/tests/YGNodeCallbackTest.cpp b/tests/YGNodeCallbackTest.cpp
@@ -33,7 +33,7 @@ TEST(Node, measure_with_measure_fn) {
 
   n.setMeasureFunc(
       [](YGNodeRef, float w, YGMeasureMode wm, float h, YGMeasureMode hm) {
-        return YGSize{w * static_cast<int>(wm), h / static_cast<int>(hm)};
+        return YGSize{w * static_cast<float>(wm), h / static_cast<float>(hm)};
       });
 
   ASSERT_EQ(