From e6b4d952e757ab99a0b67e516235b7f91904dca0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= Date: Wed, 7 Sep 2022 12:32:52 +0200 Subject: [PATCH] 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. --- doc/corrade-changelog.dox | 3 +++ src/Corrade/Utility/Configuration.cpp | 6 ------ src/Corrade/Utility/Configuration.h | 2 -- src/Corrade/Utility/ConfigurationGroup.cpp | 14 ++++++++++---- src/Corrade/Utility/ConfigurationGroup.h | 4 ++++ 5 files changed, 17 insertions(+), 12 deletions(-) diff --git a/doc/corrade-changelog.dox b/doc/corrade-changelog.dox index 14170ef3c..b2f7fb955 100644 --- 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 @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 index 2f473ed55..99941f253 100644 --- 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 index 69de1bff4..2310b80f4 100644 --- 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, const char*> parse(Containers::ArrayView 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 index d6980dbd7..73f0acc05 100644 --- 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 index c14776355..2eeee67c9 100644 --- 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::iterator findGroup(const std::string& name, unsigned int index); CORRADE_UTILITY_LOCAL std::vector::const_iterator findGroup(const std::string& name, unsigned int index) const;