From e4bc69a0f6f768fbe1d361c3315afdce0cb428cb Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Sun, 10 Sep 2023 03:31:34 -0700 Subject: [PATCH] Breaking: YGConfigRef related const-correctness fixes (#1371) Summary: X-link: https://github.com/facebook/react-native/pull/39374 Pull Request resolved: https://github.com/facebook/yoga/pull/1371 Right now `YGConfigGetDefault` and `YGNodeGetConfig` both return mutable, freeable, configs, which is bad, since the former points to a global singleton config, and the latter usually does too. Mutating this is not thread safe, and it should never be freed. This change makes these functions return `YGConfigConstRef` to prevent mutation, and also lets us allow `YGConfigNewWithConfig` to accept a const config. If a caller does want to mutate a config (such as to free it), it must be tracked manually. Changelog: [Internal] Differential Revision: D49132476 fbshipit-source-id: 374eec0f826df9e81a24b07ed701375ce99352f9 --- yoga/Yoga.cpp | 14 +++++--------- yoga/Yoga.h | 6 +++--- yoga/config/Config.cpp | 18 +++++++++++++----- yoga/config/Config.h | 8 ++++++-- yoga/debug/Log.cpp | 9 +++++---- yoga/event/event.h | 4 ++-- yoga/node/Node.cpp | 6 ++++-- yoga/node/Node.h | 10 ++++------ 8 files changed, 42 insertions(+), 33 deletions(-) diff --git a/yoga/Yoga.cpp b/yoga/Yoga.cpp index 03b4bdf943..141c4c7466 100644 --- a/yoga/Yoga.cpp +++ b/yoga/Yoga.cpp @@ -32,7 +32,7 @@ YOGA_EXPORT void YGNodeSetContext(YGNodeRef node, void* context) { return resolveRef(node)->setContext(context); } -YOGA_EXPORT YGConfigRef YGNodeGetConfig(YGNodeRef node) { +YOGA_EXPORT YGConfigConstRef YGNodeGetConfig(YGNodeRef node) { return resolveRef(node)->getConfig(); } @@ -103,22 +103,18 @@ YOGA_EXPORT void YGNodeMarkDirtyAndPropagateToDescendants( return resolveRef(node)->markDirtyAndPropagateDownwards(); } -YOGA_EXPORT WIN_EXPORT YGNodeRef YGNodeNewWithConfig(const YGConfigRef config) { +YOGA_EXPORT WIN_EXPORT YGNodeRef +YGNodeNewWithConfig(const YGConfigConstRef config) { auto* node = new yoga::Node{resolveRef(config)}; yoga::assertFatal( config != nullptr, "Tried to construct YGNode with null config"); - yoga::assertFatalWithConfig( - resolveRef(config), - node != nullptr, - "Could not allocate memory for node"); Event::publish(node, {config}); return node; } -YOGA_EXPORT YGConfigRef YGConfigGetDefault() { - static YGConfigRef defaultConfig = YGConfigNew(); - return defaultConfig; +YOGA_EXPORT YGConfigConstRef YGConfigGetDefault() { + return &yoga::Config::getDefault(); } YOGA_EXPORT YGNodeRef YGNodeNew(void) { diff --git a/yoga/Yoga.h b/yoga/Yoga.h index 2697bb40ef..208f76ada1 100644 --- a/yoga/Yoga.h +++ b/yoga/Yoga.h @@ -52,7 +52,7 @@ typedef YGNodeRef (*YGCloneNodeFunc)( // YGNode WIN_EXPORT YGNodeRef YGNodeNew(void); -WIN_EXPORT YGNodeRef YGNodeNewWithConfig(YGConfigRef config); +WIN_EXPORT YGNodeRef YGNodeNewWithConfig(YGConfigConstRef config); WIN_EXPORT YGNodeRef YGNodeClone(YGNodeConstRef node); WIN_EXPORT void YGNodeFree(YGNodeRef node); WIN_EXPORT void YGNodeFreeRecursiveWithCleanupFunc( @@ -131,7 +131,7 @@ WIN_EXPORT void YGNodeCopyStyle(YGNodeRef dstNode, YGNodeConstRef srcNode); WIN_EXPORT void* YGNodeGetContext(YGNodeConstRef node); WIN_EXPORT void YGNodeSetContext(YGNodeRef node, void* context); -WIN_EXPORT YGConfigRef YGNodeGetConfig(YGNodeRef node); +WIN_EXPORT YGConfigConstRef YGNodeGetConfig(YGNodeRef node); WIN_EXPORT void YGNodeSetConfig(YGNodeRef node, YGConfigRef config); void YGConfigSetPrintTreeFlag(YGConfigRef config, bool enabled); @@ -327,7 +327,7 @@ WIN_EXPORT void YGConfigSetCloneNodeFunc( YGConfigRef config, YGCloneNodeFunc callback); -WIN_EXPORT YGConfigRef YGConfigGetDefault(void); +WIN_EXPORT YGConfigConstRef YGConfigGetDefault(void); WIN_EXPORT void YGConfigSetContext(YGConfigRef config, void* context); WIN_EXPORT void* YGConfigGetContext(YGConfigConstRef config); diff --git a/yoga/config/Config.cpp b/yoga/config/Config.cpp index f4dea8433c..f3b28ffae0 100644 --- a/yoga/config/Config.cpp +++ b/yoga/config/Config.cpp @@ -6,15 +6,18 @@ */ #include +#include #include namespace facebook::yoga { -bool configUpdateInvalidatesLayout(Config* a, Config* b) { - return a->getErrata() != b->getErrata() || - a->getEnabledExperiments() != b->getEnabledExperiments() || - a->getPointScaleFactor() != b->getPointScaleFactor() || - a->useWebDefaults() != b->useWebDefaults(); +bool configUpdateInvalidatesLayout( + const Config& oldConfig, + const Config& newConfig) { + return oldConfig.getErrata() != newConfig.getErrata() || + oldConfig.getEnabledExperiments() != newConfig.getEnabledExperiments() || + oldConfig.getPointScaleFactor() != newConfig.getPointScaleFactor() || + oldConfig.useWebDefaults() != newConfig.useWebDefaults(); } Config::Config(YGLogger logger) : cloneNodeCallback_{nullptr} { @@ -145,4 +148,9 @@ YGNodeRef Config::cloneNode( return clone; } +/*static*/ const Config& Config::getDefault() { + static Config config{getDefaultLogger()}; + return config; +} + } // namespace facebook::yoga diff --git a/yoga/config/Config.h b/yoga/config/Config.h index d06b1bacf7..168bd7e890 100644 --- a/yoga/config/Config.h +++ b/yoga/config/Config.h @@ -18,9 +18,11 @@ namespace facebook::yoga { class Config; class Node; -// Whether moving a node from config "a" to config "b" should dirty previously +// Whether moving a node from an old to new config should dirty previously // calculated layout results. -bool configUpdateInvalidatesLayout(Config* a, Config* b); +bool configUpdateInvalidatesLayout( + const Config& oldConfig, + const Config& newConfig); // Internal variants of log functions, currently used only by JNI bindings. // TODO: Reconcile this with the public API @@ -95,6 +97,8 @@ class YOGA_EXPORT Config : public ::YGConfig { size_t childIndex, void* cloneContext) const; + static const Config& getDefault(); + private: union { CloneWithContextFn withContext; diff --git a/yoga/debug/Log.cpp b/yoga/debug/Log.cpp index 507214c793..675c1ebd4c 100644 --- a/yoga/debug/Log.cpp +++ b/yoga/debug/Log.cpp @@ -22,10 +22,11 @@ void vlog( void* context, const char* format, va_list args) { - const yoga::Config* logConfig = - config != nullptr ? config : resolveRef(YGConfigGetDefault()); - - logConfig->log(const_cast(node), level, context, format, args); + if (config == nullptr) { + getDefaultLogger()(nullptr, node, level, format, args); + } else { + config->log(node, level, context, format, args); + } } } // namespace diff --git a/yoga/event/event.h b/yoga/event/event.h index fca12aca63..f9a8bc19dc 100644 --- a/yoga/event/event.h +++ b/yoga/event/event.h @@ -95,12 +95,12 @@ struct YOGA_EXPORT Event { template <> struct Event::TypedData { - YGConfigRef config; + YGConfigConstRef config; }; template <> struct Event::TypedData { - YGConfigRef config; + YGConfigConstRef config; }; template <> diff --git a/yoga/node/Node.cpp b/yoga/node/Node.cpp index 005542dae1..b9a4da12ca 100644 --- a/yoga/node/Node.cpp +++ b/yoga/node/Node.cpp @@ -17,7 +17,9 @@ namespace facebook::yoga { -Node::Node(yoga::Config* config) : config_{config} { +Node::Node() : Node{&Config::getDefault()} {} + +Node::Node(const yoga::Config* config) : config_{config} { yoga::assertFatal( config != nullptr, "Attempting to construct Node with null config"); @@ -285,7 +287,7 @@ void Node::setConfig(yoga::Config* config) { config->useWebDefaults() == config_->useWebDefaults(), "UseWebDefaults may not be changed after constructing a Node"); - if (yoga::configUpdateInvalidatesLayout(config_, config)) { + if (yoga::configUpdateInvalidatesLayout(*config_, *config)) { markDirtyAndPropagate(); } diff --git a/yoga/node/Node.h b/yoga/node/Node.h index e99a376425..7f45802e69 100644 --- a/yoga/node/Node.h +++ b/yoga/node/Node.h @@ -71,7 +71,7 @@ class YOGA_EXPORT Node : public ::YGNode { size_t lineIndex_ = 0; Node* owner_ = nullptr; std::vector children_ = {}; - Config* config_; + const Config* config_; std::array resolvedDimensions_ = { {YGValueUndefined, YGValueUndefined}}; @@ -95,10 +95,8 @@ class YOGA_EXPORT Node : public ::YGNode { Node& operator=(Node&&) = default; public: - Node() : Node{static_cast(YGConfigGetDefault())} { - flags_.hasNewLayout = true; - } - explicit Node(Config* config); + Node(); + explicit Node(const Config* config); ~Node() = default; // cleanup of owner/children relationships in YGNodeFree Node(Node&&); @@ -165,7 +163,7 @@ class YOGA_EXPORT Node : public ::YGNode { size_t getChildCount() const { return children_.size(); } - Config* getConfig() const { return config_; } + const Config* getConfig() const { return config_; } bool isDirty() const { return flags_.isDirty; }