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 · dbdb0e7 · dbdb0e7
1 parent 65ae809
commit dbdb0e7
Show file tree

Hide file tree

Showing 11 changed files with 119 additions and 103 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/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/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,7 +189,7 @@ 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;

diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp
@@ -823,8 +823,8 @@ static void zeroOutLayoutRecursively(
     yoga::Node* const node,
     void* layoutContext) {
   node->getLayout() = {};
-  node->setLayoutDimension(0, 0);
-  node->setLayoutDimension(0, 1);
+  node->setLayoutDimension(0, YGDimensionWidth);
+  node->setLayoutDimension(0, YGDimensionHeight);
   node->setHasNewLayout(true);
 
   node->iterChildrenAfterCloningIfNeeded(
@@ -1488,19 +1488,19 @@ static void YGJustifyMainAxis(
           betweenMainDim +=
               yoga::maxOrDefined(
                   collectedFlexItemsValues.remainingFreeSpace, 0) /
-              (collectedFlexItemsValues.itemsOnLine - 1);
+              static_cast<float>(collectedFlexItemsValues.itemsOnLine - 1);
         }
         break;
       case YGJustifySpaceEvenly:
         // Space is distributed evenly across all elements
         leadingMainDim = collectedFlexItemsValues.remainingFreeSpace /
-            (collectedFlexItemsValues.itemsOnLine + 1);
+            static_cast<float>(collectedFlexItemsValues.itemsOnLine + 1);
         betweenMainDim += leadingMainDim;
         break;
       case YGJustifySpaceAround:
         // Space on the edges is half of the space between elements
         leadingMainDim = 0.5f * collectedFlexItemsValues.remainingFreeSpace /
-            collectedFlexItemsValues.itemsOnLine;
+            static_cast<float>(collectedFlexItemsValues.itemsOnLine);
         betweenMainDim += leadingMainDim * 2;
         break;
       case YGJustifyFlexStart:
@@ -1550,7 +1550,7 @@ static void YGJustifyMainAxis(
         if (child->marginLeadingValue(mainAxis).unit == YGUnitAuto) {
           collectedFlexItemsValues.mainDim +=
               collectedFlexItemsValues.remainingFreeSpace /
-              numberOfAutoMarginsOnCurrentLine;
+              static_cast<float>(numberOfAutoMarginsOnCurrentLine);
         }
 
         if (performLayout) {
@@ -1563,7 +1563,7 @@ static void YGJustifyMainAxis(
         if (child->marginTrailingValue(mainAxis).unit == YGUnitAuto) {
           collectedFlexItemsValues.mainDim +=
               collectedFlexItemsValues.remainingFreeSpace /
-              numberOfAutoMarginsOnCurrentLine;
+              static_cast<float>(numberOfAutoMarginsOnCurrentLine);
         }
         bool canSkipFlex =
             !performLayout && measureModeCrossDim == YGMeasureModeExactly;
@@ -1890,7 +1890,7 @@ static void calculateLayoutImpl(
   if (childCount > 1) {
     totalMainDim +=
         node->getGapForAxis(mainAxis, availableInnerCrossDim).unwrap() *
-        (childCount - 1);
+        static_cast<float>(childCount - 1);
   }
 
   const bool mainAxisOverflows =
@@ -2261,22 +2261,26 @@ static void calculateLayoutImpl(
           break;
         case YGAlignStretch:
           if (availableInnerCrossDim > totalLineCrossDim) {
-            crossDimLead = remainingAlignContentDim / lineCount;
+            crossDimLead =
+                remainingAlignContentDim / static_cast<float>(lineCount);
           }
           break;
         case YGAlignSpaceAround:
           if (availableInnerCrossDim > totalLineCrossDim) {
-            currentLead += remainingAlignContentDim / (2 * lineCount);
+            currentLead +=
+                remainingAlignContentDim / (2 * static_cast<float>(lineCount));
             if (lineCount > 1) {
-              crossDimLead = remainingAlignContentDim / lineCount;
+              crossDimLead =
+                  remainingAlignContentDim / static_cast<float>(lineCount);
             }
           } else {
             currentLead += remainingAlignContentDim / 2;
           }
           break;
         case YGAlignSpaceBetween:
           if (availableInnerCrossDim > totalLineCrossDim && lineCount > 1) {
-            crossDimLead = remainingAlignContentDim / (lineCount - 1);
+            crossDimLead =
+                remainingAlignContentDim / static_cast<float>(lineCount - 1);
           }
           break;
         case YGAlignAuto:
@@ -2843,11 +2847,11 @@ bool calculateLayoutInternal(
     layout->lastOwnerDirection = ownerDirection;
 
     if (cachedResults == nullptr) {
-      if (layout->nextCachedMeasurementsIndex + 1 >
-          (uint32_t) layoutMarkerData.maxMeasureCache) {
-        layoutMarkerData.maxMeasureCache =
-            layout->nextCachedMeasurementsIndex + 1;
-      }
+
+      layoutMarkerData.maxMeasureCache = std::max(
+          layoutMarkerData.maxMeasureCache,
+          layout->nextCachedMeasurementsIndex + 1u);
+
       if (layout->nextCachedMeasurementsIndex ==
           LayoutResults::MaxCachedMeasurements) {
         if (gPrintChanges) {

diff --git a/yoga/bits/NumericBitfield.h b/yoga/bits/NumericBitfield.h
@@ -15,12 +15,12 @@
 
 namespace facebook::yoga::details {
 
-constexpr size_t log2ceilFn(size_t n) {
+constexpr uint8_t log2ceilFn(uint8_t n) {
   return n < 1 ? 0 : (1 + log2ceilFn(n / 2));
 }
 
-constexpr int mask(size_t bitWidth, size_t index) {
-  return ((1 << bitWidth) - 1) << index;
+constexpr uint32_t mask(uint8_t bitWidth, uint8_t index) {
+  return ((1u << bitWidth) - 1u) << index;
 }
 
 } // namespace facebook::yoga::details
@@ -29,38 +29,31 @@ namespace facebook::yoga {
 
 // The number of bits necessary to represent enums defined with YG_ENUM_SEQ_DECL
 template <typename Enum>
-constexpr size_t minimumBitCount() {
+constexpr uint8_t minimumBitCount() {
   static_assert(
       enums::count<Enum>() > 0, "Enums must have at least one entries");
   return details::log2ceilFn(enums::count<Enum>() - 1);
 }
 
 template <typename Enum>
-constexpr Enum getEnumData(int flags, size_t index) {
+constexpr Enum getEnumData(uint32_t flags, uint8_t index) {
   return static_cast<Enum>(
       (flags & details::mask(minimumBitCount<Enum>(), index)) >> index);
 }
 
 template <typename Enum>
-void setEnumData(uint32_t& flags, size_t index, int newValue) {
-  flags = (flags & ~details::mask(minimumBitCount<Enum>(), index)) |
-      ((newValue << index) & (details::mask(minimumBitCount<Enum>(), index)));
-}
-
-template <typename Enum>
-void setEnumData(uint8_t& flags, size_t index, int newValue) {
+void setEnumData(uint32_t& flags, uint8_t index, uint32_t newValue) {
   flags =
       (flags &
-       ~static_cast<uint8_t>(details::mask(minimumBitCount<Enum>(), index))) |
-      ((newValue << index) &
-       (static_cast<uint8_t>(details::mask(minimumBitCount<Enum>(), index))));
+       ~static_cast<uint32_t>(details::mask(minimumBitCount<Enum>(), index))) |
+      ((newValue << index) & (details::mask(minimumBitCount<Enum>(), index)));
 }
 
-constexpr bool getBooleanData(int flags, size_t index) {
+constexpr bool getBooleanData(uint32_t flags, uint8_t index) {
   return (flags >> index) & 1;
 }
 
-inline void setBooleanData(uint8_t& flags, size_t index, bool value) {
+inline void setBooleanData(uint32_t& flags, uint8_t index, bool value) {
   if (value) {
     flags |= 1 << index;
   } else {

diff --git a/yoga/debug/NodeToString.cpp b/yoga/debug/NodeToString.cpp
@@ -33,7 +33,7 @@ static void appendFormattedString(std::string& str, const char* fmt, ...) {
   va_start(args, fmt);
   va_list argsCopy;
   va_copy(argsCopy, args);
-  std::vector<char> buf(1 + vsnprintf(NULL, 0, fmt, args));
+  std::vector<char> buf(1 + static_cast<size_t>(vsnprintf(NULL, 0, fmt, args)));
   va_end(args);
   vsnprintf(buf.data(), buf.size(), fmt, argsCopy);
   va_end(argsCopy);
@@ -96,7 +96,7 @@ static void appendEdges(
   } else {
     for (int edge = YGEdgeLeft; edge != YGEdgeAll; ++edge) {
       std::string str = key + "-" + YGEdgeToString(static_cast<YGEdge>(edge));
-      appendNumberIfNotZero(base, str, edges[edge]);
+      appendNumberIfNotZero(base, str, edges[static_cast<size_t>(edge)]);
     }
   }
 }

diff --git a/yoga/event/event.h b/yoga/event/event.h
@@ -38,7 +38,7 @@ enum struct LayoutPassReason : int {
 struct LayoutData {
   int layouts;
   int measures;
-  int maxMeasureCache;
+  uint32_t maxMeasureCache;
   int cachedLayouts;
   int cachedMeasures;
   int measureCallbacks;