Skip to content

Commit

Permalink
C++ Cleanup 2/N: Reorganize YGConfig (facebook#1348)
Browse files Browse the repository at this point in the history
Summary:
X-link: facebook/react-native#39169

Pull Request resolved: facebook#1348

## This diff

This diff adds a top level `config` directory for code related to configuring Yoga and Yoga Nodes.

The public API for config handles is `YGConfigRef`, which is forward declared to be a pointer to a struct named `YGConfig`. The existing `YGConfig` is split into `yoga::Config`, as the private C++ implementation, inheriting from `YGConfig`, a marker type represented as an empty struct. The public API continues to accept `YGConfigRef`, which continues to be `YGConfig *`, but it must be cast to its concrete internal representation at the API boundary before doing work on it.

## This stack

The organization of the C++ internals of Yoga are in need of attention.
1. Some of the C++ internals are namespaced, but others not.
2. Some of the namespaces include `detail`, but are meant to be used outside of the translation unit (FB Clang Tidy rules warn on any usage of these)
2. Most of the files are in a flat hierarchy, except for event tracing in its own folder
3. Some files and functions begin with YG, others don’t
4. Some functions are uppercase, others are not
5. Almost all of the interesting logic is in Yoga.cpp, and the file is too large to reason about
6. There are multiple grab bag files where folks put random functions they need in (Utils, BitUtils, Yoga-Internal.h)
7. There is no clear indication from file structure or type naming what is private vs not
8. Handles like `YGNodeRef` and `YGConfigRef` can be used to access internals just by importing headers

This stack does some much needed spring cleaning:
1. All non-public headers and C++ implementation details are in separate folders from the root level `yoga`. This will give us room to split up logic and add more files without too large a flat hierarchy
3. All private C++ internals are under the `facebook::yoga` namespace. Details namespaces are only ever used within the same header, as they are intended
4. Utils files are split
5. Most C++ internals drop the YG prefix
6. Most C++ internal function names are all lower camel case
7. We start to split up Yoga.cpp
8. Every header beginning with YG or at the top-level directory is public and C only, with the exception of Yoga-Internal.h which has non-public functions for bindings
9. It is not possible to use private APIs without static casting handles to internal classes

This will give us more leeway to continue splitting monolithic files, and consistent guidelines for style in new files as well.

These changes should not be breaking to any project using only public Yoga headers. This includes every usage of Yoga in fbsource except for RN Fabric which is currently tied to internals. This refactor should make that boundary clearer.

Changelog: [Internal]

Reviewed By: rshest

Differential Revision: D48710796

fbshipit-source-id: 2bae928d6a8e0fa0e354711f75d096dde7cbf720
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Aug 30, 2023
1 parent 135906a commit 8a9f429
Show file tree
Hide file tree
Showing 10 changed files with 122 additions and 107 deletions.
4 changes: 3 additions & 1 deletion java/jni/YGJNIVanilla.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
// API and use that
#include <yoga/YGNode.h>

using namespace facebook;
using namespace facebook::yoga;
using namespace facebook::yoga::vanillajni;

static inline ScopedLocalRef<jobject> YGNodeJobject(
Expand Down Expand Up @@ -194,7 +196,7 @@ static void jni_YGConfigSetLoggerJNI(
}

*context = newGlobalRef(env, logger);
config->setLogger(YGJNILogFunc);
static_cast<yoga::Config*>(config)->setLogger(YGJNILogFunc);
} else {
if (context != nullptr) {
delete context;
Expand Down
8 changes: 5 additions & 3 deletions tests/generated/YGConfigTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@

#include <gtest/gtest.h>
#include <yoga/Yoga.h>
#include <yoga/YGConfig.h>
#include <yoga/config/Config.h>
#include <yoga/YGNode.h>

#include <functional>
#include <memory>

using namespace facebook;

struct ConfigCloningTest : public ::testing::Test {
std::unique_ptr<YGConfig, std::function<void(YGConfig*)>> config;
std::unique_ptr<yoga::Config, std::function<void(yoga::Config*)>> config;
void SetUp() override;
void TearDown() override;

Expand Down Expand Up @@ -56,7 +58,7 @@ TEST_F(ConfigCloningTest, can_clone_with_context) {
}

void ConfigCloningTest::SetUp() {
config = {YGConfigNew(), YGConfigFree};
config = {static_cast<yoga::Config*>(YGConfigNew()), YGConfigFree};
}

void ConfigCloningTest::TearDown() {
Expand Down
4 changes: 2 additions & 2 deletions yoga/YGNode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ using namespace facebook;
using namespace facebook::yoga;
using facebook::yoga::CompactValue;

YGNode::YGNode(const YGConfigRef config) : config_{config} {
YGNode::YGNode(yoga::Config* config) : config_{config} {
YGAssert(
config != nullptr, "Attempting to construct YGNode with null config");

Expand Down Expand Up @@ -264,7 +264,7 @@ void YGNode::insertChild(YGNodeRef child, uint32_t index) {
children_.insert(children_.begin() + index, child);
}

void YGNode::setConfig(YGConfigRef config) {
void YGNode::setConfig(yoga::Config* config) {
YGAssert(config != nullptr, "Attempting to set a null config on a YGNode");
YGAssertWithConfig(
config,
Expand Down
17 changes: 9 additions & 8 deletions yoga/YGNode.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,13 @@

#include <cstdint>
#include <stdio.h>
#include "YGConfig.h"
#include <yoga/config/Config.h>
#include "YGLayout.h"
#include <yoga/Yoga-internal.h>

#include <yoga/style/CompactValue.h>
#include <yoga/style/Style.h>

YGConfigRef YGConfigGetDefault();

#pragma pack(push)
#pragma pack(1)
struct YGNodeFlags {
Expand Down Expand Up @@ -58,7 +56,7 @@ struct YOGA_EXPORT YGNode {
uint32_t lineIndex_ = 0;
YGNodeRef owner_ = nullptr;
YGVector children_ = {};
YGConfigRef config_;
facebook::yoga::Config* config_;
std::array<YGValue, 2> resolvedDimensions_ = {
{YGValueUndefined, YGValueUndefined}};

Expand All @@ -84,8 +82,11 @@ struct YOGA_EXPORT YGNode {
using CompactValue = facebook::yoga::CompactValue;

public:
YGNode() : YGNode{YGConfigGetDefault()} { flags_.hasNewLayout = true; }
explicit YGNode(const YGConfigRef config);
YGNode()
: YGNode{static_cast<facebook::yoga::Config*>(YGConfigGetDefault())} {
flags_.hasNewLayout = true;
}
explicit YGNode(facebook::yoga::Config* config);
~YGNode() = default; // cleanup of owner/children relationships in YGNodeFree

YGNode(YGNode&&);
Expand Down Expand Up @@ -166,7 +167,7 @@ struct YOGA_EXPORT YGNode {

YGNodeRef getChild(uint32_t index) const { return children_.at(index); }

YGConfigRef getConfig() const { return config_; }
facebook::yoga::Config* getConfig() const { return config_; }

bool isDirty() const { return flags_.isDirty; }

Expand Down Expand Up @@ -290,7 +291,7 @@ struct YOGA_EXPORT YGNode {

// TODO: rvalue override for setChildren

void setConfig(YGConfigRef config);
void setConfig(facebook::yoga::Config* config);

void setDirty(bool isDirty);
void setLayoutLastOwnerDirection(YGDirection direction);
Expand Down
73 changes: 39 additions & 34 deletions yoga/Yoga.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <yoga/Yoga-internal.h>
#include "event/event.h"

using namespace facebook;
using namespace facebook::yoga;
using detail::Log;

Expand Down Expand Up @@ -119,7 +120,7 @@ YOGA_EXPORT YGConfigRef YGNodeGetConfig(YGNodeRef node) {
}

YOGA_EXPORT void YGNodeSetConfig(YGNodeRef node, YGConfigRef config) {
node->setConfig(config);
node->setConfig(static_cast<yoga::Config*>(config));
}

YOGA_EXPORT bool YGNodeHasMeasureFunc(YGNodeRef node) {
Expand Down Expand Up @@ -161,7 +162,7 @@ YOGA_EXPORT bool YGNodeGetHasNewLayout(YGNodeRef node) {
}

YOGA_EXPORT void YGConfigSetPrintTreeFlag(YGConfigRef config, bool enabled) {
config->setShouldPrintTree(enabled);
static_cast<yoga::Config*>(config)->setShouldPrintTree(enabled);
}

YOGA_EXPORT void YGNodeSetHasNewLayout(YGNodeRef node, bool hasNewLayout) {
Expand All @@ -188,7 +189,7 @@ YOGA_EXPORT void YGNodeMarkDirtyAndPropagateToDescendants(
int32_t gConfigInstanceCount = 0;

YOGA_EXPORT WIN_EXPORT YGNodeRef YGNodeNewWithConfig(const YGConfigRef config) {
const YGNodeRef node = new YGNode{config};
const YGNodeRef node = new YGNode{static_cast<yoga::Config*>(config)};
YGAssert(config != nullptr, "Tried to construct YGNode with null config");
YGAssertWithConfig(
config, node != nullptr, "Could not allocate memory for node");
Expand Down Expand Up @@ -272,23 +273,19 @@ YOGA_EXPORT int32_t YGConfigGetInstanceCount(void) {

YOGA_EXPORT YGConfigRef YGConfigNew(void) {
#ifdef ANDROID
const YGConfigRef config = new YGConfig(YGAndroidLog);
const YGConfigRef config = new yoga::Config(YGAndroidLog);
#else
const YGConfigRef config = new YGConfig(YGDefaultLog);
const YGConfigRef config = new yoga::Config(YGDefaultLog);
#endif
gConfigInstanceCount++;
return config;
}

YOGA_EXPORT void YGConfigFree(const YGConfigRef config) {
delete config;
delete static_cast<yoga::Config*>(config);
gConfigInstanceCount--;
}

void YGConfigCopy(const YGConfigRef dest, const YGConfigRef src) {
memcpy(dest, src, sizeof(YGConfig));
}

YOGA_EXPORT void YGNodeSetIsReferenceBaseline(
YGNodeRef node,
bool isReferenceBaseline) {
Expand Down Expand Up @@ -3715,22 +3712,22 @@ YOGA_EXPORT bool YGNodeCanUseCachedMeasurement(
return false;
}
bool useRoundedComparison =
config != nullptr && config->getPointScaleFactor() != 0;
config != nullptr && YGConfigGetPointScaleFactor(config) != 0;
const float effectiveWidth = useRoundedComparison
? YGRoundValueToPixelGrid(
width, config->getPointScaleFactor(), false, false)
width, YGConfigGetPointScaleFactor(config), false, false)
: width;
const float effectiveHeight = useRoundedComparison
? YGRoundValueToPixelGrid(
height, config->getPointScaleFactor(), false, false)
height, YGConfigGetPointScaleFactor(config), false, false)
: height;
const float effectiveLastWidth = useRoundedComparison
? YGRoundValueToPixelGrid(
lastWidth, config->getPointScaleFactor(), false, false)
lastWidth, YGConfigGetPointScaleFactor(config), false, false)
: lastWidth;
const float effectiveLastHeight = useRoundedComparison
? YGRoundValueToPixelGrid(
lastHeight, config->getPointScaleFactor(), false, false)
lastHeight, YGConfigGetPointScaleFactor(config), false, false)
: lastHeight;

const bool hasSameWidthSpec = lastWidthMode == widthMode &&
Expand Down Expand Up @@ -4057,14 +4054,14 @@ YOGA_EXPORT void YGConfigSetPointScaleFactor(
// We store points for Pixel as we will use it for rounding
if (pixelsInPoint == 0.0f) {
// Zero is used to skip rounding
config->setPointScaleFactor(0.0f);
static_cast<yoga::Config*>(config)->setPointScaleFactor(0.0f);
} else {
config->setPointScaleFactor(pixelsInPoint);
static_cast<yoga::Config*>(config)->setPointScaleFactor(pixelsInPoint);
}
}

YOGA_EXPORT float YGConfigGetPointScaleFactor(const YGConfigRef config) {
return config->getPointScaleFactor();
return static_cast<yoga::Config*>(config)->getPointScaleFactor();
}

static void YGRoundToPixelGrid(
Expand Down Expand Up @@ -4239,12 +4236,12 @@ YOGA_EXPORT void YGNodeCalculateLayout(

YOGA_EXPORT void YGConfigSetLogger(const YGConfigRef config, YGLogger logger) {
if (logger != nullptr) {
config->setLogger(logger);
static_cast<yoga::Config*>(config)->setLogger(logger);
} else {
#ifdef ANDROID
config->setLogger(&YGAndroidLog);
static_cast<yoga::Config*>(config)->setLogger(&YGAndroidLog);
#else
config->setLogger(&YGDefaultLog);
static_cast<yoga::Config*>(config)->setLogger(&YGDefaultLog);
#endif
}
}
Expand All @@ -4271,7 +4268,12 @@ void YGAssertWithConfig(
const bool condition,
const char* message) {
if (!condition) {
Log::log(config, YGLogLevelFatal, nullptr, "%s\n", message);
Log::log(
static_cast<yoga::Config*>(config),
YGLogLevelFatal,
nullptr,
"%s\n",
message);
throwLogicalErrorWithMessage(message);
}
}
Expand All @@ -4280,58 +4282,61 @@ YOGA_EXPORT void YGConfigSetExperimentalFeatureEnabled(
const YGConfigRef config,
const YGExperimentalFeature feature,
const bool enabled) {
config->setExperimentalFeatureEnabled(feature, enabled);
static_cast<yoga::Config*>(config)->setExperimentalFeatureEnabled(
feature, enabled);
}

YOGA_EXPORT bool YGConfigIsExperimentalFeatureEnabled(
const YGConfigRef config,
const YGExperimentalFeature feature) {
return config->isExperimentalFeatureEnabled(feature);
return static_cast<yoga::Config*>(config)->isExperimentalFeatureEnabled(
feature);
}

YOGA_EXPORT void YGConfigSetUseWebDefaults(
const YGConfigRef config,
const bool enabled) {
config->setUseWebDefaults(enabled);
static_cast<yoga::Config*>(config)->setUseWebDefaults(enabled);
}

YOGA_EXPORT bool YGConfigGetUseLegacyStretchBehaviour(
const YGConfigRef config) {
return config->hasErrata(YGErrataStretchFlexBasis);
return static_cast<yoga::Config*>(config)->hasErrata(
YGErrataStretchFlexBasis);
}

YOGA_EXPORT void YGConfigSetUseLegacyStretchBehaviour(
const YGConfigRef config,
const bool useLegacyStretchBehaviour) {
if (useLegacyStretchBehaviour) {
config->addErrata(YGErrataStretchFlexBasis);
static_cast<yoga::Config*>(config)->addErrata(YGErrataStretchFlexBasis);
} else {
config->removeErrata(YGErrataStretchFlexBasis);
static_cast<yoga::Config*>(config)->removeErrata(YGErrataStretchFlexBasis);
}
}

bool YGConfigGetUseWebDefaults(const YGConfigRef config) {
return config->useWebDefaults();
return static_cast<yoga::Config*>(config)->useWebDefaults();
}

YOGA_EXPORT void YGConfigSetContext(const YGConfigRef config, void* context) {
config->setContext(context);
static_cast<yoga::Config*>(config)->setContext(context);
}

YOGA_EXPORT void* YGConfigGetContext(const YGConfigRef config) {
return config->getContext();
return static_cast<yoga::Config*>(config)->getContext();
}

YOGA_EXPORT void YGConfigSetErrata(YGConfigRef config, YGErrata errata) {
config->setErrata(errata);
static_cast<yoga::Config*>(config)->setErrata(errata);
}

YOGA_EXPORT YGErrata YGConfigGetErrata(YGConfigRef config) {
return config->getErrata();
return static_cast<yoga::Config*>(config)->getErrata();
}

YOGA_EXPORT void YGConfigSetCloneNodeFunc(
const YGConfigRef config,
const YGCloneNodeFunc callback) {
config->setCloneNodeCallback(callback);
static_cast<yoga::Config*>(config)->setCloneNodeCallback(callback);
}
1 change: 0 additions & 1 deletion yoga/Yoga.h
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,6 @@ void YGConfigSetUseLegacyStretchBehaviour(
// YGConfig
WIN_EXPORT YGConfigRef YGConfigNew(void);
WIN_EXPORT void YGConfigFree(YGConfigRef config);
WIN_EXPORT void YGConfigCopy(YGConfigRef dest, YGConfigRef src);
WIN_EXPORT int32_t YGConfigGetInstanceCount(void);

WIN_EXPORT void YGConfigSetExperimentalFeatureEnabled(
Expand Down
Loading

0 comments on commit 8a9f429

Please sign in to comment.