Skip to content

refactor: make ConfigBase subclass constructors private #368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wgtmac merged 2 commits into from
Dec 1, 2025
Merged

refactor: make ConfigBase subclass constructors private #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ccf9c7
refactor: make ConfigBase subclass constructors private
HeartLinked Nov 28, 2025
f11d3ec
update
HeartLinked Nov 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/iceberg/file_reader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ Result<std::unique_ptr<Reader>> ReaderFactoryRegistry::Open(
}

std::unique_ptr<ReaderProperties> ReaderProperties::default_properties() {
return std::make_unique<ReaderProperties>();
return std::unique_ptr<ReaderProperties>(new ReaderProperties());
Copy link
Contributor

@shangxinli shangxinli Nov 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this specific case is actually safe because it is single expression, immediately returned and no other allocations happening. But I thought we generally prefer to use make_unique for it's safety.

Copy link
Contributor

@HuaHuaY HuaHuaY Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::make_unique needs a public constructor, so it can't be used when constructor is marked to private.

}

std::unique_ptr<ReaderProperties> ReaderProperties::FromMap(
const std::unordered_map<std::string, std::string>& properties) {
auto reader_properties = std::make_unique<ReaderProperties>();
auto reader_properties = std::unique_ptr<ReaderProperties>(new ReaderProperties());
reader_properties->configs_ = properties;
return reader_properties;
}
Expand Down
3 changes: 3 additions & 0 deletions src/iceberg/file_reader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@ class ReaderProperties : public ConfigBase<ReaderProperties> {
/// \brief Create a ReaderProperties instance from a map of key-value pairs.
static std::unique_ptr<ReaderProperties> FromMap(
const std::unordered_map<std::string, std::string>& properties);

private:
ReaderProperties() = default;
};

/// \brief Options for creating a reader.
Expand Down
4 changes: 2 additions & 2 deletions src/iceberg/file_writer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ Result<std::unique_ptr<Writer>> WriterFactoryRegistry::Open(
}

std::unique_ptr<WriterProperties> WriterProperties::default_properties() {
return std::make_unique<WriterProperties>();
return std::unique_ptr<WriterProperties>(new WriterProperties());
}

std::unique_ptr<WriterProperties> WriterProperties::FromMap(
const std::unordered_map<std::string, std::string>& properties) {
auto writer_properties = std::make_unique<WriterProperties>();
auto writer_properties = std::unique_ptr<WriterProperties>(new WriterProperties());
writer_properties->configs_ = properties;
return writer_properties;
}
Expand Down
3 changes: 3 additions & 0 deletions src/iceberg/file_writer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ class WriterProperties : public ConfigBase<WriterProperties> {
/// \brief Create a WriterProperties instance from a map of key-value pairs.
static std::unique_ptr<WriterProperties> FromMap(
const std::unordered_map<std::string, std::string>& properties);

private:
WriterProperties() = default;
};

/// \brief Options for creating a writer.
Expand Down
4 changes: 2 additions & 2 deletions src/iceberg/table_properties.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ const std::unordered_set<std::string>& TableProperties::reserved_properties() {
}

std::unique_ptr<TableProperties> TableProperties::default_properties() {
return std::make_unique<TableProperties>();
return std::unique_ptr<TableProperties>(new TableProperties());
}

std::unique_ptr<TableProperties> TableProperties::FromMap(
const std::unordered_map<std::string, std::string>& properties) {
auto table_properties = std::make_unique<TableProperties>();
auto table_properties = std::unique_ptr<TableProperties>(new TableProperties());
for (const auto& [key, value] : properties) { // NOLINT(modernize-type-traits)
table_properties->configs_[key] = value;
}
Expand Down
3 changes: 3 additions & 0 deletions src/iceberg/table_properties.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,9 @@ class ICEBERG_EXPORT TableProperties : public ConfigBase<TableProperties> {
/// \return A unique pointer to a TableProperties instance
static std::unique_ptr<TableProperties> FromMap(
const std::unordered_map<std::string, std::string>& properties);

private:
TableProperties() = default;
};

} // namespace iceberg
69 changes: 38 additions & 31 deletions src/iceberg/test/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,48 +64,55 @@ class TestConfig : public ConfigBase<TestConfig> {
inline static const Entry<TestEnum> kEnumConfig{"enum_config", TestEnum::VALUE1,
EnumToString, StringToEnum};
inline static const Entry<double> kDoubleConfig{"double_config", 3.14};

static std::unique_ptr<TestConfig> default_properties() {
return std::unique_ptr<TestConfig>(new TestConfig());
}

private:
TestConfig() = default;
};

TEST(ConfigTest, BasicOperations) {
TestConfig config;
auto config = TestConfig::default_properties();

// Test default values
ASSERT_EQ(config.Get(TestConfig::kStringConfig), std::string("default_value"));
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
ASSERT_EQ(config->Get(TestConfig::kStringConfig), std::string("default_value"));
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);

// Test setting values
config.Set(TestConfig::kStringConfig, std::string("new_value"));
config.Set(TestConfig::kIntConfig, 100);
config.Set(TestConfig::kBoolConfig, true);
config.Set(TestConfig::kEnumConfig, TestEnum::VALUE2);
config.Set(TestConfig::kDoubleConfig, 2.99);

ASSERT_EQ(config.Get(TestConfig::kStringConfig), "new_value");
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 100);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), true);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE2);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 2.99);
config->Set(TestConfig::kStringConfig, std::string("new_value"));
config->Set(TestConfig::kIntConfig, 100);
config->Set(TestConfig::kBoolConfig, true);
config->Set(TestConfig::kEnumConfig, TestEnum::VALUE2);
config->Set(TestConfig::kDoubleConfig, 2.99);

ASSERT_EQ(config->Get(TestConfig::kStringConfig), "new_value");
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 100);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), true);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE2);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 2.99);

// Test unsetting a value
config.Unset(TestConfig::kIntConfig);
config.Unset(TestConfig::kEnumConfig);
config.Unset(TestConfig::kDoubleConfig);
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "new_value");
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), true);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
config->Unset(TestConfig::kIntConfig);
config->Unset(TestConfig::kEnumConfig);
config->Unset(TestConfig::kDoubleConfig);
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "new_value");
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), true);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);

// Test resetting all values
config.Reset();
ASSERT_EQ(config.Get(TestConfig::kStringConfig), "default_value");
ASSERT_EQ(config.Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config.Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config.Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config.Get(TestConfig::kDoubleConfig), 3.14);
config->Reset();
ASSERT_EQ(config->Get(TestConfig::kStringConfig), "default_value");
ASSERT_EQ(config->Get(TestConfig::kIntConfig), 25);
ASSERT_EQ(config->Get(TestConfig::kBoolConfig), false);
ASSERT_EQ(config->Get(TestConfig::kEnumConfig), TestEnum::VALUE1);
ASSERT_EQ(config->Get(TestConfig::kDoubleConfig), 3.14);
}

} // namespace iceberg
Loading