Skip to content

Commit

Permalink
Breaking: size_t indices (#1366)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #1366

X-link: facebook/react-native#39371

Yoga's public API exposes indices most often as `uint32_t`, with exception of clone callbacks which are `int32_t`. Yoga internally represents these indices as `size_t` when dealing with the child vector, and this is the true index.

This changes the API to consistently be `size_t`. This should not be breaking for most users, but will cause breaks where:

1. Users set a clone node callback (I think this should be rare. RN uses it, but only because it relies on a separate private API).
2. Callers of `YGNodeGetChildCount()` are assigning to an int with less width than `size_t` and have strong warnings enabled.
3. Using a newer Yoga binary with older source, since we are not preserving ABI compatibility (Yoga in general does not aim to be ABI stable between major versions, only ABI safe for a given set of sources).

Changelog: [Internal]

Reviewed By: sammy-SC

Differential Revision: D49130914

fbshipit-source-id: 6b9de766af3a9ccdc8772f60a1671c31d934ae91
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Sep 11, 2023
1 parent b1db416 commit 9e23366
Show file tree
Hide file tree
Showing 14 changed files with 60 additions and 66 deletions.
2 changes: 1 addition & 1 deletion java/jni/YGJNIVanilla.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ static void YGTransferLayoutOutputsRecursive(

YGNodeSetHasNewLayout(root, false);

for (uint32_t i = 0; i < YGNodeGetChildCount(root); i++) {
for (size_t i = 0; i < YGNodeGetChildCount(root); i++) {
YGTransferLayoutOutputsRecursive(
env, thiz, YGNodeGetChild(root, i), layoutContext);
}
Expand Down
8 changes: 3 additions & 5 deletions tests/generated/YGConfigTest.cpp → tests/YGConfigTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
* LICENSE file in the root directory of this source tree.
*/

// @Generated by gentest/gentest.rb from gentest/fixtures/YGDisplayTest.html

#include <gtest/gtest.h>
#include <yoga/Yoga.h>
#include <yoga/config/Config.h>
Expand All @@ -23,10 +21,10 @@ struct ConfigCloningTest : public ::testing::Test {
void TearDown() override;

static yoga::Node clonedNode;
static YGNodeRef cloneNode(YGNodeConstRef, YGNodeConstRef, int) {
static YGNodeRef cloneNode(YGNodeConstRef, YGNodeConstRef, size_t) {
return &clonedNode;
}
static YGNodeRef doNotClone(YGNodeConstRef, YGNodeConstRef, int) {
static YGNodeRef doNotClone(YGNodeConstRef, YGNodeConstRef, size_t) {
return nullptr;
}
};
Expand Down Expand Up @@ -54,7 +52,7 @@ TEST_F(

TEST_F(ConfigCloningTest, can_clone_with_context) {
config->setCloneNodeCallback(
[](YGNodeConstRef, YGNodeConstRef, int, void* context) {
[](YGNodeConstRef, YGNodeConstRef, size_t, void* context) {
return (YGNodeRef) context;
});

Expand Down
4 changes: 2 additions & 2 deletions tests/YGTreeMutationTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
#include <yoga/Yoga.h>

static std::vector<YGNodeRef> getChildren(YGNodeRef const node) {
const uint32_t count = YGNodeGetChildCount(node);
const auto count = YGNodeGetChildCount(node);
std::vector<YGNodeRef> children;
children.reserve(count);
for (uint32_t i = 0; i < count; i++) {
for (size_t i = 0; i < count; i++) {
children.push_back(YGNodeGetChild(node, i));
}
return children;
Expand Down
30 changes: 15 additions & 15 deletions yoga/Yoga.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -216,8 +216,8 @@ YOGA_EXPORT void YGNodeFree(const YGNodeRef nodeRef) {
node->setOwner(nullptr);
}

const uint32_t childCount = YGNodeGetChildCount(node);
for (uint32_t i = 0; i < childCount; i++) {
const size_t childCount = node->getChildCount();
for (size_t i = 0; i < childCount; i++) {
auto child = node->getChild(i);
child->setOwner(nullptr);
}
Expand All @@ -236,8 +236,8 @@ YOGA_EXPORT void YGNodeFreeRecursiveWithCleanupFunc(
YGNodeCleanupFunc cleanup) {
const auto root = resolveRef(rootRef);

uint32_t skipped = 0;
while (YGNodeGetChildCount(root) > skipped) {
size_t skipped = 0;
while (root->getChildCount() > skipped) {
const auto child = root->getChild(skipped);
if (child->getOwner() != root) {
// Don't free shared nodes that we don't own.
Expand Down Expand Up @@ -297,7 +297,7 @@ YOGA_EXPORT bool YGNodeIsReferenceBaseline(YGNodeConstRef node) {
YOGA_EXPORT void YGNodeInsertChild(
const YGNodeRef ownerRef,
const YGNodeRef childRef,
const uint32_t index) {
const size_t index) {
auto owner = resolveRef(ownerRef);
auto child = resolveRef(childRef);

Expand All @@ -319,7 +319,7 @@ YOGA_EXPORT void YGNodeInsertChild(
YOGA_EXPORT void YGNodeSwapChild(
const YGNodeRef ownerRef,
const YGNodeRef childRef,
const uint32_t index) {
const size_t index) {
auto owner = resolveRef(ownerRef);
auto child = resolveRef(childRef);

Expand All @@ -333,7 +333,7 @@ YOGA_EXPORT void YGNodeRemoveChild(
auto owner = resolveRef(ownerRef);
auto excludedChild = resolveRef(excludedChildRef);

if (YGNodeGetChildCount(owner) == 0) {
if (owner->getChildCount() == 0) {
// This is an empty set. Nothing to remove.
return;
}
Expand All @@ -354,7 +354,7 @@ YOGA_EXPORT void YGNodeRemoveChild(
YOGA_EXPORT void YGNodeRemoveAllChildren(const YGNodeRef ownerRef) {
auto owner = resolveRef(ownerRef);

const uint32_t childCount = YGNodeGetChildCount(owner);
const size_t childCount = owner->getChildCount();
if (childCount == 0) {
// This is an empty set already. Nothing to do.
return;
Expand All @@ -363,7 +363,7 @@ YOGA_EXPORT void YGNodeRemoveAllChildren(const YGNodeRef ownerRef) {
if (firstChild->getOwner() == owner) {
// If the first child has this node as its owner, we assume that this child
// set is unique.
for (uint32_t i = 0; i < childCount; i++) {
for (size_t i = 0; i < childCount; i++) {
yoga::Node* oldChild = owner->getChild(i);
oldChild->setLayout({}); // layout is no longer valid
oldChild->setOwner(nullptr);
Expand All @@ -381,7 +381,7 @@ YOGA_EXPORT void YGNodeRemoveAllChildren(const YGNodeRef ownerRef) {
YOGA_EXPORT void YGNodeSetChildren(
const YGNodeRef ownerRef,
const YGNodeRef* childrenRefs,
const uint32_t count) {
const size_t count) {
auto owner = resolveRef(ownerRef);
auto children = reinterpret_cast<yoga::Node* const*>(childrenRefs);

Expand All @@ -391,7 +391,7 @@ YOGA_EXPORT void YGNodeSetChildren(

const std::vector<yoga::Node*> childrenVector = {children, children + count};
if (childrenVector.size() == 0) {
if (YGNodeGetChildCount(owner) > 0) {
if (owner->getChildCount() > 0) {
for (auto* child : owner->getChildren()) {
child->setLayout({});
child->setOwner(nullptr);
Expand All @@ -400,7 +400,7 @@ YOGA_EXPORT void YGNodeSetChildren(
owner->markDirtyAndPropagate();
}
} else {
if (YGNodeGetChildCount(owner) > 0) {
if (owner->getChildCount() > 0) {
for (auto* oldChild : owner->getChildren()) {
// Our new children may have nodes in common with the old children. We
// don't reset these common nodes.
Expand All @@ -420,7 +420,7 @@ YOGA_EXPORT void YGNodeSetChildren(
}

YOGA_EXPORT YGNodeRef
YGNodeGetChild(const YGNodeRef nodeRef, const uint32_t index) {
YGNodeGetChild(const YGNodeRef nodeRef, const size_t index) {
const auto node = resolveRef(nodeRef);

if (index < node->getChildren().size()) {
Expand All @@ -429,8 +429,8 @@ YGNodeGetChild(const YGNodeRef nodeRef, const uint32_t index) {
return nullptr;
}

YOGA_EXPORT uint32_t YGNodeGetChildCount(const YGNodeConstRef node) {
return static_cast<uint32_t>(resolveRef(node)->getChildren().size());
YOGA_EXPORT size_t YGNodeGetChildCount(const YGNodeConstRef node) {
return resolveRef(node)->getChildren().size();
}

YOGA_EXPORT YGNodeRef YGNodeGetOwner(const YGNodeRef node) {
Expand Down
16 changes: 7 additions & 9 deletions yoga/Yoga.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

#include <stdarg.h>
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>

#include <yoga/YGEnums.h>
Expand Down Expand Up @@ -47,7 +48,7 @@ typedef int (*YGLogger)(
typedef YGNodeRef (*YGCloneNodeFunc)(
YGNodeConstRef oldNode,
YGNodeConstRef owner,
int childIndex);
size_t childIndex);

// YGNode
WIN_EXPORT YGNodeRef YGNodeNew(void);
Expand All @@ -63,23 +64,20 @@ WIN_EXPORT void YGNodeReset(YGNodeRef node);
WIN_EXPORT void YGNodeInsertChild(
YGNodeRef node,
YGNodeRef child,
uint32_t index);
size_t index);

WIN_EXPORT void YGNodeSwapChild(
YGNodeRef node,
YGNodeRef child,
uint32_t index);
WIN_EXPORT void YGNodeSwapChild(YGNodeRef node, YGNodeRef child, size_t index);

WIN_EXPORT void YGNodeRemoveChild(YGNodeRef node, YGNodeRef child);
WIN_EXPORT void YGNodeRemoveAllChildren(YGNodeRef node);
WIN_EXPORT YGNodeRef YGNodeGetChild(YGNodeRef node, uint32_t index);
WIN_EXPORT YGNodeRef YGNodeGetChild(YGNodeRef node, size_t index);
WIN_EXPORT YGNodeRef YGNodeGetOwner(YGNodeRef node);
WIN_EXPORT YGNodeRef YGNodeGetParent(YGNodeRef node);
WIN_EXPORT uint32_t YGNodeGetChildCount(YGNodeConstRef node);
WIN_EXPORT size_t YGNodeGetChildCount(YGNodeConstRef node);
WIN_EXPORT void YGNodeSetChildren(
YGNodeRef owner,
const YGNodeRef* children,
uint32_t count);
size_t count);

WIN_EXPORT void YGNodeSetIsReferenceBaseline(
YGNodeRef node,
Expand Down
4 changes: 2 additions & 2 deletions yoga/algorithm/Baseline.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ float calculateBaseline(const yoga::Node* node, void* layoutContext) {
}

yoga::Node* baselineChild = nullptr;
const uint32_t childCount = YGNodeGetChildCount(node);
for (uint32_t i = 0; i < childCount; i++) {
const size_t childCount = node->getChildCount();
for (size_t i = 0; i < childCount; i++) {
auto child = node->getChild(i);
if (child->getLineIndex() > 0) {
break;
Expand Down
36 changes: 17 additions & 19 deletions yoga/algorithm/CalculateLayout.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -965,8 +965,8 @@ static CollectFlexItemsRowValues calculateCollectFlexItemsRowValues(
const float mainAxisownerSize,
const float availableInnerWidth,
const float availableInnerMainDim,
const uint32_t startOfLineIndex,
const uint32_t lineCount) {
const size_t startOfLineIndex,
const size_t lineCount) {
CollectFlexItemsRowValues flexAlgoRowMeasurement = {};
flexAlgoRowMeasurement.relativeChildren.reserve(node->getChildren().size());

Expand All @@ -977,7 +977,7 @@ static CollectFlexItemsRowValues calculateCollectFlexItemsRowValues(
const float gap = node->getGapForAxis(mainAxis, availableInnerWidth).unwrap();

// Add items to the current line until it's full or we run out of items.
uint32_t endOfLineIndex = startOfLineIndex;
size_t endOfLineIndex = startOfLineIndex;
for (; endOfLineIndex < node->getChildren().size(); endOfLineIndex++) {
auto child = node->getChild(endOfLineIndex);
if (child->getStyle().display() == YGDisplayNone ||
Expand Down Expand Up @@ -1408,7 +1408,7 @@ static void resolveFlexibleLength(
static void YGJustifyMainAxis(
yoga::Node* const node,
CollectFlexItemsRowValues& collectedFlexItemsValues,
const uint32_t startOfLineIndex,
const size_t startOfLineIndex,
const YGFlexDirection mainAxis,
const YGFlexDirection crossAxis,
const YGMeasureMode measureModeMainDim,
Expand Down Expand Up @@ -1456,8 +1456,7 @@ static void YGJustifyMainAxis(
}

int numberOfAutoMarginsOnCurrentLine = 0;
for (uint32_t i = startOfLineIndex;
i < collectedFlexItemsValues.endOfLineIndex;
for (size_t i = startOfLineIndex; i < collectedFlexItemsValues.endOfLineIndex;
i++) {
auto child = node->getChild(i);
if (child->getStyle().positionType() != YGPositionTypeAbsolute) {
Expand Down Expand Up @@ -1517,8 +1516,7 @@ static void YGJustifyMainAxis(
float maxAscentForCurrentLine = 0;
float maxDescentForCurrentLine = 0;
bool isNodeBaselineLayout = isBaselineLayout(node);
for (uint32_t i = startOfLineIndex;
i < collectedFlexItemsValues.endOfLineIndex;
for (size_t i = startOfLineIndex; i < collectedFlexItemsValues.endOfLineIndex;
i++) {
const auto child = node->getChild(i);
const Style& childStyle = child->getStyle();
Expand Down Expand Up @@ -1906,11 +1904,11 @@ static void calculateLayoutImpl(
// STEP 4: COLLECT FLEX ITEMS INTO FLEX LINES

// Indexes of children that represent the first and last items in the line.
uint32_t startOfLineIndex = 0;
uint32_t endOfLineIndex = 0;
size_t startOfLineIndex = 0;
size_t endOfLineIndex = 0;

// Number of lines.
uint32_t lineCount = 0;
size_t lineCount = 0;

// Accumulated cross dimensions of all lines so far.
float totalLineCrossDim = 0;
Expand Down Expand Up @@ -2093,7 +2091,7 @@ static void calculateLayoutImpl(
// STEP 7: CROSS-AXIS ALIGNMENT
// We can skip child alignment if we're just measuring the container.
if (performLayout) {
for (uint32_t i = startOfLineIndex; i < endOfLineIndex; i++) {
for (size_t i = startOfLineIndex; i < endOfLineIndex; i++) {
const auto child = node->getChild(i);
if (child->getStyle().display() == YGDisplayNone) {
continue;
Expand Down Expand Up @@ -2291,10 +2289,10 @@ static void calculateLayoutImpl(
break;
}
}
uint32_t endIndex = 0;
for (uint32_t i = 0; i < lineCount; i++) {
const uint32_t startIndex = endIndex;
uint32_t ii;
size_t endIndex = 0;
for (size_t i = 0; i < lineCount; i++) {
const size_t startIndex = endIndex;
size_t ii;

// compute the line's height and find the endIndex
float lineHeight = 0;
Expand Down Expand Up @@ -2537,7 +2535,7 @@ static void calculateLayoutImpl(
// As we only wrapped in normal direction yet, we need to reverse the
// positions on wrap-reverse.
if (performLayout && node->getStyle().flexWrap() == YGWrapWrapReverse) {
for (uint32_t i = 0; i < childCount; i++) {
for (size_t i = 0; i < childCount; i++) {
const auto child = node->getChild(i);
if (child->getStyle().positionType() != YGPositionTypeAbsolute) {
child->setLayoutPosition(
Expand Down Expand Up @@ -2586,7 +2584,7 @@ static void calculateLayoutImpl(

// Set trailing position if necessary.
if (needsMainTrailingPos || needsCrossTrailingPos) {
for (uint32_t i = 0; i < childCount; i++) {
for (size_t i = 0; i < childCount; i++) {
const auto child = node->getChild(i);
if (child->getStyle().display() == YGDisplayNone) {
continue;
Expand Down Expand Up @@ -2710,7 +2708,7 @@ bool calculateLayoutInternal(
cachedResults = &layout->cachedLayout;
} else {
// Try to use the measurement cache.
for (uint32_t i = 0; i < layout->nextCachedMeasurementsIndex; i++) {
for (size_t i = 0; i < layout->nextCachedMeasurementsIndex; i++) {
if (canUseCachedMeasurement(
widthMeasureMode,
availableWidth,
Expand Down
4 changes: 2 additions & 2 deletions yoga/algorithm/CollectFlexItemsRowValues.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ namespace facebook::yoga {
// and/or grow.

struct CollectFlexItemsRowValues {
uint32_t itemsOnLine;
size_t itemsOnLine;
float sizeConsumedOnCurrentLine;
float totalFlexGrowFactors;
float totalFlexShrinkScaledFactors;
uint32_t endOfLineIndex;
size_t endOfLineIndex;
std::vector<yoga::Node*> relativeChildren;
float remainingFreeSpace;
// The size of the mainDim for the row after considering size, padding, margin
Expand Down
4 changes: 2 additions & 2 deletions yoga/algorithm/PixelGrid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,8 @@ void roundLayoutResultsToPixelGrid(
absoluteNodeTop, pointScaleFactor, false, textRounding),
YGDimensionHeight);

const uint32_t childCount = YGNodeGetChildCount(node);
for (uint32_t i = 0; i < childCount; i++) {
const size_t childCount = node->getChildCount();
for (size_t i = 0; i < childCount; i++) {
roundLayoutResultsToPixelGrid(
node->getChild(i), pointScaleFactor, absoluteNodeLeft, absoluteNodeTop);
}
Expand Down
2 changes: 1 addition & 1 deletion yoga/config/Config.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ void Config::setCloneNodeCallback(std::nullptr_t) {
YGNodeRef Config::cloneNode(
YGNodeConstRef node,
YGNodeConstRef owner,
int childIndex,
size_t childIndex,
void* cloneContext) const {
YGNodeRef clone = nullptr;
if (cloneNodeCallback_.noContext != nullptr) {
Expand Down
4 changes: 2 additions & 2 deletions yoga/config/Config.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ using LogWithContextFn = int (*)(
using CloneWithContextFn = YGNodeRef (*)(
YGNodeConstRef node,
YGNodeConstRef owner,
int childIndex,
size_t childIndex,
void* cloneContext);

#pragma pack(push)
Expand Down Expand Up @@ -92,7 +92,7 @@ class YOGA_EXPORT Config : public ::YGConfig {
YGNodeRef cloneNode(
YGNodeConstRef node,
YGNodeConstRef owner,
int childIndex,
size_t childIndex,
void* cloneContext) const;

private:
Expand Down
Loading

0 comments on commit 9e23366

Please sign in to comment.