Skip to content

Commit c41d749

Browse files
nathanhhughesnathanhhughes
authored
Fix/default config bug (#60)
* add failing test for nested virtual configs * simplify unit test * fix cmake deprecation warning * always convey config type even if not set
1 parent baa7acf commit c41d749

File tree

4 files changed

+109
-18
lines changed

4 files changed

+109
-18
lines changed

‎.pre-commit-config.yaml‎

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,11 @@ repos:
1010
- id: check-added-large-files
1111
- id: check-merge-conflict
1212

13-
- repo: https://github.com/pre-commit/mirrors-yapf
14-
rev: 'v0.32.0'
13+
- repo: https://github.com/google/yapf
14+
rev: 'v0.43.0'
1515
hooks:
1616
- id: yapf
1717

18-
- repo: https://github.com/pylint-dev/pylint
19-
rev: 'v3.0.0a6'
20-
hooks:
21-
- id: pylint
22-
2318
- repo: https://github.com/pre-commit/mirrors-clang-format
2419
rev: 'v16.0.3'
2520
hooks:

‎config_utilities/CMakeLists.txt‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@ if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
99
add_compile_options(-Wall -Wextra -Wpedantic)
1010
endif()
1111

12-
1312
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
14-
1513
include(OptionalPackage)
1614

1715
option(BUILD_SHARED_LIBS "Build shared libs" ON)
@@ -21,7 +19,7 @@ option(CONFIG_UTILS_BUILD_TESTS "Build unit tests" ON)
2119
option(CONFIG_UTILS_BUILD_DEMOS "Build demo executables" ON)
2220

2321
find_package(yaml-cpp REQUIRED)
24-
find_package(Boost REQUIRED COMPONENTS filesystem system)
22+
find_package(Boost CONFIG REQUIRED COMPONENTS filesystem system)
2523
find_optional(Eigen3 CONFIG_UTILS_ENABLE_EIGEN)
2624
find_optional_pkgcfg(libglog CONFIG_UTILS_ENABLE_GLOG)
2725

@@ -50,7 +48,7 @@ add_library(
5048
src/visitor.cpp
5149
src/yaml_parser.cpp
5250
src/yaml_utils.cpp
53-
)
51+
)
5452
target_link_libraries(
5553
${PROJECT_NAME}
5654
PUBLIC yaml-cpp
@@ -86,4 +84,4 @@ if(CONFIG_UTILS_BUILD_TESTS)
8684
add_subdirectory(test)
8785
endif()
8886

89-
include(HandleInstall)
87+
include(HandleInstall)

‎config_utilities/src/visitor.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -99,11 +99,13 @@ std::optional<YAML::Node> Visitor::visitVirtualConfig(bool is_set,
9999
}
100100
}
101101

102-
if (is_set && (visitor.mode == Visitor::Mode::kGet || visitor.mode == Visitor::Mode::kGetInfo)) {
102+
if (visitor.mode == Visitor::Mode::kGet || visitor.mode == Visitor::Mode::kGetInfo) {
103103
// Also write the type param back to file.
104104
std::string error;
105-
YAML::Node type_node =
106-
YamlParser::toYaml(Settings::instance().factory.type_param_name, type, visitor.name_space, error);
105+
YAML::Node type_node = YamlParser::toYaml(Settings::instance().factory.type_param_name,
106+
is_set ? type : kUninitializedVirtualConfigType,
107+
visitor.name_space,
108+
error);
107109
mergeYamlNodes(visitor.data.data, type_node);
108110
}
109111

‎config_utilities/test/tests/virtual_config.cpp‎

Lines changed: 99 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ struct Derived2WithComplexParam : public Base2 {
9696
const std::shared_ptr<int> i_;
9797
inline static const auto registration_ =
9898
config::RegistrationWithConfig<Base2, Derived2WithComplexParam, Config, std::shared_ptr<int>>(
99-
"Derived2WithComplexParam");
99+
"Derived2WithComplexParam");
100100
};
101101

102102
void declare_config(Derived2WithComplexParam::Config& config) {
@@ -108,13 +108,14 @@ struct Derived2WithMoveOnlyParam : public Base2 {
108108
struct Config {
109109
int i = 0;
110110
};
111-
explicit Derived2WithMoveOnlyParam(const Config& config, std::unique_ptr<int> i) : config_(config), i_(std::move(i)) {}
111+
explicit Derived2WithMoveOnlyParam(const Config& config, std::unique_ptr<int> i)
112+
: config_(config), i_(std::move(i)) {}
112113
std::string name() const override { return "Derived2WithMoveOnlyParam"; }
113114
const Config config_;
114115
const std::unique_ptr<int> i_;
115116
inline static const auto registration_ =
116117
config::RegistrationWithConfig<Base2, Derived2WithMoveOnlyParam, Config, std::unique_ptr<int>>(
117-
"Derived2WithMoveOnlyParam");
118+
"Derived2WithMoveOnlyParam");
118119
};
119120

120121
void declare_config(Derived2WithMoveOnlyParam::Config& config) {
@@ -174,6 +175,48 @@ void declare_config(ObjectWithOptionalConfigs::Config& config) {
174175
config::field(config.modules, "modules");
175176
}
176177

178+
struct DefaultedOptional {
179+
struct Config {
180+
int foo = 3;
181+
} const config;
182+
explicit DefaultedOptional(const Config& config) : config(config) {}
183+
};
184+
185+
void declare_config(DefaultedOptional::Config& config) {
186+
name<DefaultedOptional::Config>();
187+
field(config.foo, "foo");
188+
}
189+
190+
struct ParentOfDefaultedOptional {
191+
struct Config {
192+
VirtualConfig<DefaultedOptional> child{DefaultedOptional::Config()};
193+
} const config;
194+
195+
explicit ParentOfDefaultedOptional(const Config& config) : config(config), child(config.child.create()) {}
196+
std::unique_ptr<DefaultedOptional> child;
197+
};
198+
199+
void declare_config(ParentOfDefaultedOptional::Config& config) {
200+
name<ParentOfDefaultedOptional::Config>();
201+
field(config.child, "child");
202+
config.child.setOptional();
203+
}
204+
205+
struct GrandparentOfDefaultedOptional {
206+
struct Config {
207+
VirtualConfig<ParentOfDefaultedOptional> child;
208+
} const config;
209+
210+
explicit GrandparentOfDefaultedOptional(const Config& config) : config(config), child(config.child.create()) {}
211+
std::unique_ptr<ParentOfDefaultedOptional> child;
212+
};
213+
214+
void declare_config(GrandparentOfDefaultedOptional::Config& config) {
215+
name<ParentOfDefaultedOptional::Config>();
216+
field(config.child, "child");
217+
config.child.setOptional();
218+
}
219+
177220
TEST(VirtualConfig, isSet) {
178221
Settings().restoreDefaults();
179222

@@ -570,4 +613,57 @@ TEST(VirtualConfig, optionalNullCreation) {
570613
ASSERT_EQ(object, nullptr);
571614
}
572615

616+
TEST(VirtualConfig, defaultedConfigCorrect) {
617+
RegistrationGuard<DefaultedOptional, DefaultedOptional, DefaultedOptional::Config> guard("DefaultedOptional");
618+
RegistrationGuard<ParentOfDefaultedOptional, ParentOfDefaultedOptional, ParentOfDefaultedOptional::Config>
619+
parent_guard("ParentOfDefaultedOptional");
620+
RegistrationGuard<GrandparentOfDefaultedOptional,
621+
GrandparentOfDefaultedOptional,
622+
GrandparentOfDefaultedOptional::Config>
623+
grandparent_guard("GrandparentOfDefaultedOptional");
624+
625+
{ // default config does the right thing from YAML
626+
const auto node = YAML::Load(R"""(
627+
type: GrandparentOfDefaultedOptional
628+
child:
629+
type: ParentOfDefaultedOptional
630+
)""");
631+
auto root = config::createFromYaml<GrandparentOfDefaultedOptional>(node);
632+
ASSERT_TRUE(root);
633+
ASSERT_TRUE(root->child);
634+
EXPECT_TRUE(root->child->child);
635+
}
636+
637+
{ // manually specifying the type does the right thing
638+
const auto node = YAML::Load(R"""(
639+
type: GrandparentOfDefaultedOptional
640+
child:
641+
type: ParentOfDefaultedOptional
642+
child:
643+
type: DefaultedOptional
644+
foo: 5
645+
)""");
646+
auto root = config::createFromYaml<GrandparentOfDefaultedOptional>(node);
647+
ASSERT_TRUE(root);
648+
ASSERT_TRUE(root->child);
649+
ASSERT_TRUE(root->child->child);
650+
EXPECT_EQ(root->child->child->config.foo, 5);
651+
}
652+
653+
{ // overriding default does the right thing
654+
const auto node = YAML::Load(R"""(
655+
type: GrandparentOfDefaultedOptional
656+
child:
657+
type: ParentOfDefaultedOptional
658+
child:
659+
type: ''
660+
)""");
661+
auto root_config = config::fromYaml<GrandparentOfDefaultedOptional::Config>(node);
662+
auto root = config::createFromYaml<GrandparentOfDefaultedOptional>(node);
663+
ASSERT_TRUE(root);
664+
ASSERT_TRUE(root->child);
665+
EXPECT_FALSE(root->child->child);
666+
}
667+
}
668+
573669
} // namespace config::test

0 commit comments

Comments
 (0)