Skip to content

Commit 70bc6d2

Browse files
committed
fix(json_wrapper): address review comments
- Initialize json_ in member initializer list for exception safety - Add missing #include <string> in test file - Fix Copy Non-owning Wrapper test to verify deep copy by scoping w2 outside w1 Signed-off-by: LHT129 <tianlan.lht@antgroup.com>
1 parent 9a98496 commit 70bc6d2

2 files changed

Lines changed: 7 additions & 10 deletions

File tree

src/json_wrapper.cpp

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,9 @@ JsonWrapper::~JsonWrapper() {
3030
}
3131
}
3232

33-
JsonWrapper::JsonWrapper(const JsonWrapper& other) : owns_json_(true) {
34-
if (other.json_ != nullptr) {
35-
json_ = new nlohmann::json(*other.json_);
36-
} else {
37-
json_ = new nlohmann::json();
38-
}
33+
JsonWrapper::JsonWrapper(const JsonWrapper& other)
34+
: json_(other.json_ ? new nlohmann::json(*other.json_) : new nlohmann::json()),
35+
owns_json_(true) {
3936
}
4037

4138
JsonWrapper&

tests/test_json_wrapper.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// limitations under the License.
1414

1515
#include <catch2/catch_test_macros.hpp>
16+
#include <string>
1617

1718
#include "json_wrapper.h"
1819

@@ -53,12 +54,11 @@ TEST_CASE("JsonWrapper Copy Nested Json", "[ft][json_wrapper]") {
5354
}
5455

5556
TEST_CASE("JsonWrapper Copy Non-owning Wrapper", "[ft][json_wrapper]") {
56-
std::string copied_value;
57+
vsag::JsonWrapper w2 = vsag::JsonWrapper();
5758
{
5859
auto w1 = vsag::JsonWrapper::Parse(R"({"key": "value"})");
5960
vsag::JsonWrapper sub = w1["key"];
60-
vsag::JsonWrapper w2 = sub;
61-
copied_value = w2.GetString();
61+
w2 = sub;
6262
}
63-
CHECK(copied_value == "value");
63+
CHECK(w2.GetString() == "value");
6464
}

0 commit comments

Comments
 (0)