Utility: fix config pointer reassign in deep ConfigurationGroups.

It did so only for the first level, but not for deeper nesting. Both failed tests pass now; interestingly enough such recursive configuration pointer reset was done correctly in the Configuration itself, but not in the base -- so that's now unified. But effectively this pointer shouldn't need to be maintained at all, it's just one extra moving part that will go out of sync and the only purpose of it is to mark the configuration as changed. Instead the configuration itself should go over the groups on save and check on its own if a "modified" bit is set. 2007 me wasn't a good programmer it seems.
mosra · Sep 7, 2022 · e6b4d95 · e6b4d95
1 parent 3e6bc77
commit e6b4d95
Show file tree

Hide file tree

Showing 5 changed files with 17 additions and 12 deletions.
diff --git a/doc/corrade-changelog.dox b/doc/corrade-changelog.dox
@@ -417,6 +417,9 @@ namespace Corrade {
     zero (see [mosra/corrade#102](https://github.com/mosra/corrade/issues/102))
 -   Fixed @ref Utility::Arguments to produce an error instead of asserting in
     case a positional argument is specified as named
+-   Deep @ref Utility::ConfigurationGroup assignments didn't properly reassign
+    the @relativeref{Utility::ConfigurationGroup,configuration()} pointer,
+    leading to an assertion on save
 -   @ref Corrade/Utility/StlForwardTuple.h was fixed to do a full
     @cpp #include <tuple> @ce on libstdc++ before version 7 as there a forward
     declaration is not available

diff --git a/src/Corrade/Utility/Configuration.cpp b/src/Corrade/Utility/Configuration.cpp
@@ -90,12 +90,6 @@ Configuration& Configuration::operator=(Configuration&& other) {
     return *this;
 }
 
-void Configuration::setConfigurationPointer(ConfigurationGroup* group) {
-    group->_configuration = this;
-
-    for(auto& g: group->_groups) setConfigurationPointer(g.group);
-}
-
 std::string Configuration::filename() const { return _filename; }
 
 void Configuration::setFilename(std::string filename) {

diff --git a/src/Corrade/Utility/Configuration.h b/src/Corrade/Utility/Configuration.h
@@ -372,8 +372,6 @@ class CORRADE_UTILITY_EXPORT Configuration: public ConfigurationGroup {
         CORRADE_UTILITY_LOCAL std::pair<Containers::ArrayView<const char>, const char*> parse(Containers::ArrayView<const char> in, ConfigurationGroup* group, const std::string& fullPath);
         CORRADE_UTILITY_LOCAL void save(std::ostream& out, const std::string& eol, ConfigurationGroup* group, const std::string& fullPath) const;
 
-        CORRADE_UTILITY_LOCAL void setConfigurationPointer(ConfigurationGroup* group);
-
         std::string _filename;
         InternalFlags _flags;
 };

diff --git a/src/Corrade/Utility/ConfigurationGroup.cpp b/src/Corrade/Utility/ConfigurationGroup.cpp
@@ -80,6 +80,12 @@ ConfigurationGroup::Values::Values(const Value* begin, const Value* end, bool sk
 
 ConfigurationGroup::ConfigurationGroup(): _configuration(nullptr) {}
 
+void ConfigurationGroup::setConfigurationPointer(Configuration* configuration) {
+    _configuration = configuration;
+    for(Group& group: _groups)
+        group.group->setConfigurationPointer(configuration);
+}
+
 ConfigurationGroup::ConfigurationGroup(Configuration* configuration): _configuration(configuration) {}
 
 ConfigurationGroup::ConfigurationGroup(const ConfigurationGroup& other): _values(other._values), _groups(other._groups), _configuration(nullptr) {
@@ -91,7 +97,7 @@ ConfigurationGroup::ConfigurationGroup(const ConfigurationGroup& other): _values
 ConfigurationGroup::ConfigurationGroup(ConfigurationGroup&& other): _values(std::move(other._values)), _groups(std::move(other._groups)), _configuration(nullptr) {
     /* Reset configuration pointer for subgroups */
     for(Group& group: _groups)
-        group.group->_configuration = nullptr;
+        group.group->setConfigurationPointer(nullptr);
 }
 
 ConfigurationGroup& ConfigurationGroup::operator=(const ConfigurationGroup& other) {
@@ -106,7 +112,7 @@ ConfigurationGroup& ConfigurationGroup::operator=(const ConfigurationGroup& othe
     /* Deep copy groups */
     for(Group& group: _groups) {
         group.group = new ConfigurationGroup(*group.group);
-        group.group->_configuration = _configuration;
+        group.group->setConfigurationPointer(_configuration);
     }
 
     return *this;
@@ -123,7 +129,7 @@ ConfigurationGroup& ConfigurationGroup::operator=(ConfigurationGroup&& other) {
 
     /* Redirect configuration pointer for subgroups */
     for(Group& group: _groups)
-        group.group->_configuration = _configuration;
+        group.group->setConfigurationPointer(_configuration);
 
     return *this;
 }
@@ -205,7 +211,7 @@ void ConfigurationGroup::addGroup(const std::string& name, ConfigurationGroup* g
     /* Set configuration pointer to actual */
     CORRADE_ASSERT(!group->_configuration,
         "Utility::Configuration::addGroup(): the group is already part of some configuration", );
-    group->_configuration = _configuration;
+    group->setConfigurationPointer(_configuration);
 
     CORRADE_ASSERT(!name.empty(),
         "Utility::ConfigurationGroup::addGroup(): empty group name", );

diff --git a/src/Corrade/Utility/ConfigurationGroup.h b/src/Corrade/Utility/ConfigurationGroup.h
@@ -476,7 +476,11 @@ class CORRADE_UTILITY_EXPORT ConfigurationGroup {
             ConfigurationGroup* group;
         };
 
+        /* Used by Configuration constructor */
         CORRADE_UTILITY_LOCAL explicit ConfigurationGroup(Configuration* configuration);
+        /* Used by operator=() and addGroup(ConfigurationGroup*), and by
+           Configuration */
+        CORRADE_UTILITY_LOCAL void setConfigurationPointer(Configuration* configuration);
 
         CORRADE_UTILITY_LOCAL std::vector<Group>::iterator findGroup(const std::string& name, unsigned int index);
         CORRADE_UTILITY_LOCAL std::vector<Group>::const_iterator findGroup(const std::string& name, unsigned int index) const;