Breaking: YGConfigRef related const-correctness fixes (facebook#1371)

Summary: X-link: facebook/react-native#39374 Pull Request resolved: facebook#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
NickGerleman · Sep 10, 2023 · e4bc69a · e4bc69a
1 parent c48bb4e
commit e4bc69a
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 33 deletions.
diff --git 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<Event::NodeAllocation>(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
@@ -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
@@ -6,15 +6,18 @@
  */
 
 #include <yoga/config/Config.h>
+#include <yoga/debug/Log.h>
 #include <yoga/node/Node.h>
 
 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
@@ -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
@@ -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<yoga::Node*>(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
@@ -95,12 +95,12 @@ struct YOGA_EXPORT Event {
 
 template <>
 struct Event::TypedData<Event::NodeAllocation> {
-  YGConfigRef config;
+  YGConfigConstRef config;
 };
 
 template <>
 struct Event::TypedData<Event::NodeDeallocation> {
-  YGConfigRef config;
+  YGConfigConstRef config;
 };
 
 template <>

diff --git 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
@@ -71,7 +71,7 @@ class YOGA_EXPORT Node : public ::YGNode {
   size_t lineIndex_ = 0;
   Node* owner_ = nullptr;
   std::vector<Node*> children_ = {};
-  Config* config_;
+  const Config* config_;
   std::array<YGValue, 2> resolvedDimensions_ = {
       {YGValueUndefined, YGValueUndefined}};
 
@@ -95,10 +95,8 @@ class YOGA_EXPORT Node : public ::YGNode {
   Node& operator=(Node&&) = default;
 
 public:
-  Node() : Node{static_cast<Config*>(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; }