Skip to content

Commit 825e482

Browse files
yingcai-cyyingcai-cy
committed
fix review issus.
1 parent 7d5c901 commit 825e482

File tree

2 files changed

+47
-44
lines changed

2 files changed

+47
-44
lines changed

‎src/iceberg/util/config.h‎

Lines changed: 18 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,13 @@
2323
#include <string>
2424
#include <unordered_map>
2525

26-
namespace iceberg {
26+
#include <iceberg/exception.h>
2727

28+
namespace iceberg {
29+
namespace internal {
2830
// Default conversion functions
2931
template <typename U>
30-
std::string defaultToString(const U& val) {
32+
std::string DefaultToString(const U& val) {
3133
if constexpr ((std::is_signed_v<U> && std::is_integral_v<U>) ||
3234
std::is_floating_point_v<U>) {
3335
return std::to_string(val);
@@ -37,13 +39,13 @@ std::string defaultToString(const U& val) {
3739
std::is_same_v<U, std::string_view>) {
3840
return val;
3941
} else {
40-
throw std::runtime_error(
41-
std::format("Explicit toStr() is required for {}", typeid(U).name()));
42+
throw IcebergError(
43+
std::format("Explicit to_str() is required for {}", typeid(U).name()));
4244
}
4345
}
4446

4547
template <typename U>
46-
U defaultFromString(const std::string& val) {
48+
U DefaultFromString(const std::string& val) {
4749
if constexpr (std::is_same_v<U, std::string>) {
4850
return val;
4951
} else if constexpr (std::is_same_v<U, bool>) {
@@ -53,10 +55,11 @@ U defaultFromString(const std::string& val) {
5355
} else if constexpr (std::is_floating_point_v<U>) {
5456
return static_cast<U>(std::stod(val));
5557
} else {
56-
throw std::runtime_error(
57-
std::format("Explicit toT() is required for {}", typeid(U).name()));
58+
throw IcebergError(
59+
std::format("Explicit from_str() is required for {}", typeid(U).name()));
5860
}
5961
}
62+
} // namespace internal
6063

6164
template <class ConcreteConfig>
6265
class ConfigBase {
@@ -65,8 +68,8 @@ class ConfigBase {
6568
class Entry {
6669
public:
6770
Entry(std::string key, const T& val,
68-
std::function<std::string(const T&)> to_str = defaultToString<T>,
69-
std::function<T(const std::string&)> from_str = defaultFromString<T>)
71+
std::function<std::string(const T&)> to_str = internal::DefaultToString<T>,
72+
std::function<T(const std::string&)> from_str = internal::DefaultFromString<T>)
7073
: key_{std::move(key)}, default_{val}, to_str_{to_str}, from_str_{from_str} {}
7174

7275
private:
@@ -85,29 +88,29 @@ class ConfigBase {
8588
};
8689

8790
template <typename T>
88-
ConfigBase& set(const Entry<T>& entry, const T& val) {
89-
configs_[entry.key_] = entry.to_str_(val);
91+
ConfigBase& Set(const Entry<T>& entry, const T& val) {
92+
configs_.emplace(entry.key_, entry.to_str_(val));
9093
return *this;
9194
}
9295

9396
template <typename T>
94-
ConfigBase& unset(const Entry<T>& entry) {
97+
ConfigBase& Unset(const Entry<T>& entry) {
9598
configs_.erase(entry.key_);
9699
return *this;
97100
}
98101

99-
ConfigBase& reset() {
102+
ConfigBase& Reset() {
100103
configs_.clear();
101104
return *this;
102105
}
103106

104107
template <typename T>
105-
T get(const Entry<T>& entry) const {
108+
T Get(const Entry<T>& entry) const {
106109
auto iter = configs_.find(entry.key_);
107110
return iter != configs_.cend() ? entry.from_str_(iter->second) : entry.default_;
108111
}
109112

110-
std::unordered_map<std::string, std::string> const& configs() const { return configs_; }
113+
const std::unordered_map<std::string, std::string>& configs() const { return configs_; }
111114

112115
protected:
113116
std::unordered_map<std::string, std::string> configs_;

‎test/config_test.cc‎

Lines changed: 29 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -69,42 +69,42 @@ TEST(ConfigTest, BasicOperations) {
6969
TestConfig config;
7070

7171
// Test default values
72-
ASSERT_EQ(config.get(TestConfig::STRING_CONFIG), std::string("default_value"));
73-
ASSERT_EQ(config.get(TestConfig::INT_CONFIG), 25);
74-
ASSERT_EQ(config.get(TestConfig::BOOL_CONFIG), false);
75-
ASSERT_EQ(config.get(TestConfig::ENUM_CONFIG), TestEnum::VALUE1);
76-
ASSERT_EQ(config.get(TestConfig::DOUBLE_CONFIG), 3.14);
72+
ASSERT_EQ(config.Get(TestConfig::STRING_CONFIG), std::string("default_value"));
73+
ASSERT_EQ(config.Get(TestConfig::INT_CONFIG), 25);
74+
ASSERT_EQ(config.Get(TestConfig::BOOL_CONFIG), false);
75+
ASSERT_EQ(config.Get(TestConfig::ENUM_CONFIG), TestEnum::VALUE1);
76+
ASSERT_EQ(config.Get(TestConfig::DOUBLE_CONFIG), 3.14);
7777

7878
// Test setting values
79-
config.set(TestConfig::STRING_CONFIG, std::string("new_value"));
80-
config.set(TestConfig::INT_CONFIG, 100);
81-
config.set(TestConfig::BOOL_CONFIG, true);
82-
config.set(TestConfig::ENUM_CONFIG, TestEnum::VALUE2);
83-
config.set(TestConfig::DOUBLE_CONFIG, 2.99);
79+
config.Set(TestConfig::STRING_CONFIG, std::string("new_value"));
80+
config.Set(TestConfig::INT_CONFIG, 100);
81+
config.Set(TestConfig::BOOL_CONFIG, true);
82+
config.Set(TestConfig::ENUM_CONFIG, TestEnum::VALUE2);
83+
config.Set(TestConfig::DOUBLE_CONFIG, 2.99);
8484

85-
ASSERT_EQ(config.get(TestConfig::STRING_CONFIG), "new_value");
86-
ASSERT_EQ(config.get(TestConfig::INT_CONFIG), 100);
87-
ASSERT_EQ(config.get(TestConfig::BOOL_CONFIG), true);
88-
ASSERT_EQ(config.get(TestConfig::ENUM_CONFIG), TestEnum::VALUE2);
89-
ASSERT_EQ(config.get(TestConfig::DOUBLE_CONFIG), 2.99);
85+
ASSERT_EQ(config.Get(TestConfig::STRING_CONFIG), "new_value");
86+
ASSERT_EQ(config.Get(TestConfig::INT_CONFIG), 100);
87+
ASSERT_EQ(config.Get(TestConfig::BOOL_CONFIG), true);
88+
ASSERT_EQ(config.Get(TestConfig::ENUM_CONFIG), TestEnum::VALUE2);
89+
ASSERT_EQ(config.Get(TestConfig::DOUBLE_CONFIG), 2.99);
9090

9191
// Test unsetting a value
92-
config.unset(TestConfig::INT_CONFIG);
93-
config.unset(TestConfig::ENUM_CONFIG);
94-
config.unset(TestConfig::DOUBLE_CONFIG);
95-
ASSERT_EQ(config.get(TestConfig::INT_CONFIG), 25);
96-
ASSERT_EQ(config.get(TestConfig::STRING_CONFIG), "new_value");
97-
ASSERT_EQ(config.get(TestConfig::BOOL_CONFIG), true);
98-
ASSERT_EQ(config.get(TestConfig::ENUM_CONFIG), TestEnum::VALUE1);
99-
ASSERT_EQ(config.get(TestConfig::DOUBLE_CONFIG), 3.14);
92+
config.Unset(TestConfig::INT_CONFIG);
93+
config.Unset(TestConfig::ENUM_CONFIG);
94+
config.Unset(TestConfig::DOUBLE_CONFIG);
95+
ASSERT_EQ(config.Get(TestConfig::INT_CONFIG), 25);
96+
ASSERT_EQ(config.Get(TestConfig::STRING_CONFIG), "new_value");
97+
ASSERT_EQ(config.Get(TestConfig::BOOL_CONFIG), true);
98+
ASSERT_EQ(config.Get(TestConfig::ENUM_CONFIG), TestEnum::VALUE1);
99+
ASSERT_EQ(config.Get(TestConfig::DOUBLE_CONFIG), 3.14);
100100

101101
// Test resetting all values
102-
config.reset();
103-
ASSERT_EQ(config.get(TestConfig::STRING_CONFIG), "default_value");
104-
ASSERT_EQ(config.get(TestConfig::INT_CONFIG), 25);
105-
ASSERT_EQ(config.get(TestConfig::BOOL_CONFIG), false);
106-
ASSERT_EQ(config.get(TestConfig::ENUM_CONFIG), TestEnum::VALUE1);
107-
ASSERT_EQ(config.get(TestConfig::DOUBLE_CONFIG), 3.14);
102+
config.Reset();
103+
ASSERT_EQ(config.Get(TestConfig::STRING_CONFIG), "default_value");
104+
ASSERT_EQ(config.Get(TestConfig::INT_CONFIG), 25);
105+
ASSERT_EQ(config.Get(TestConfig::BOOL_CONFIG), false);
106+
ASSERT_EQ(config.Get(TestConfig::ENUM_CONFIG), TestEnum::VALUE1);
107+
ASSERT_EQ(config.Get(TestConfig::DOUBLE_CONFIG), 3.14);
108108
}
109109

110110
} // namespace iceberg

0 commit comments

Comments
 (0)