Skip to content

Commit 5cf60c7

Browse files
battrebattre
authored
Merge pull request #246 from cdesouza-chromium/avoid-manual-memory-management-for-string
Avoid manual lifetime management of `std::string`
2 parents f748096 + 2175a8f commit 5cf60c7

16 files changed

Lines changed: 64 additions & 83 deletions

‎cpp/include/libaddressinput/null_storage.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class NullStorage : public Storage {
3535
~NullStorage() override;
3636

3737
// No-op.
38-
void Put(const std::string& key, std::string* data) override;
38+
void Put(const std::string& key, std::string data) override;
3939

4040
// Always calls the |data_ready| callback function signaling failure.
4141
void Get(const std::string& key, const Callback& data_ready) const override;

‎cpp/include/libaddressinput/source.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
#include <libaddressinput/callback.h>
2323

24+
#include <optional>
2425
#include <string>
2526

2627
namespace i18n {
@@ -41,7 +42,7 @@ namespace addressinput {
4142
class Source {
4243
public:
4344
using Callback =
44-
i18n::addressinput::Callback<const std::string&, std::string*>;
45+
i18n::addressinput::Callback<const std::string&, std::optional<std::string>>;
4546

4647
virtual ~Source() = default;
4748

‎cpp/include/libaddressinput/storage.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
#include <libaddressinput/callback.h>
2222

2323
#include <string>
24+
#include <optional>
2425

2526
namespace i18n {
2627
namespace addressinput {
@@ -45,13 +46,13 @@ namespace addressinput {
4546
class Storage {
4647
public:
4748
using Callback =
48-
i18n::addressinput::Callback<const std::string&, std::string*>;
49+
i18n::addressinput::Callback<const std::string&, std::optional<std::string>>;
4950

5051
virtual ~Storage() = default;
5152

5253
// Stores |data| for |key|, where |data| is an object allocated on the heap,
5354
// which Storage takes ownership of.
54-
virtual void Put(const std::string& key, std::string* data) = 0;
55+
virtual void Put(const std::string& key, std::string data) = 0;
5556

5657
// Retrieves the data for |key| and invokes the |data_ready| callback.
5758
virtual void Get(const std::string& key,

‎cpp/src/null_storage.cc‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,11 @@ namespace addressinput {
2424
NullStorage::NullStorage() = default;
2525
NullStorage::~NullStorage() = default;
2626

27-
void NullStorage::Put(const std::string& key, std::string* data) {
28-
assert(data != nullptr); // Sanity check.
29-
delete data;
30-
}
27+
void NullStorage::Put(const std::string& key, std::string data) {}
3128

3229
void NullStorage::Get(const std::string& key,
3330
const Callback& data_ready) const {
34-
data_ready(false, key, nullptr);
31+
data_ready(false, key, std::nullopt);
3532
}
3633

3734
} // namespace addressinput

‎cpp/src/retriever.cc‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -56,38 +56,35 @@ class Helper {
5656

5757
void OnValidatedDataReady(bool success,
5858
const std::string& key,
59-
std::string* data) {
59+
std::optional<std::string> data) {
6060
if (success) {
61-
assert(data != nullptr);
61+
assert(data != std::nullopt);
6262
retrieved_(success, key, *data);
6363
delete this;
6464
} else {
6565
// Validating storage returns (false, key, stale-data) for valid but stale
6666
// data. If |data| is empty, however, then it's either missing or invalid.
67-
if (data != nullptr && !data->empty()) {
68-
stale_data_ = *data;
67+
if (data.has_value() && !data->empty()) {
68+
stale_data_ = std::move(data).value();
6969
}
7070
source_.Get(key, *fresh_data_ready_);
7171
}
72-
delete data;
7372
}
7473

7574
void OnFreshDataReady(bool success,
7675
const std::string& key,
77-
std::string* data) {
76+
std::optional<std::string> data) {
7877
if (success) {
79-
assert(data != nullptr);
78+
assert(data.has_value());
8079
retrieved_(true, key, *data);
81-
storage_->Put(key, data);
82-
data = nullptr; // Deleted by Storage::Put().
80+
storage_->Put(key, std::move(data).value());
8381
} else if (!stale_data_.empty()) {
8482
// Reuse the stale data if a download fails. It's better to have slightly
8583
// outdated validation rules than to suddenly lose validation ability.
8684
retrieved_(true, key, stale_data_);
8785
} else {
8886
retrieved_(false, key, std::string());
8987
}
90-
delete data;
9188
delete this;
9289
}
9390

‎cpp/src/validating_storage.cc‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,20 +51,18 @@ class Helper {
5151

5252
void OnWrappedDataReady(bool success,
5353
const std::string& key,
54-
std::string* data) {
54+
std::optional<std::string> data) {
5555
if (success) {
56-
assert(data != nullptr);
56+
assert(data != std::nullopt);
5757
bool is_stale =
58-
!ValidatingUtil::UnwrapTimestamp(data, std::time(nullptr));
59-
bool is_corrupted = !ValidatingUtil::UnwrapChecksum(data);
58+
!ValidatingUtil::UnwrapTimestamp(&*data, std::time(nullptr));
59+
bool is_corrupted = !ValidatingUtil::UnwrapChecksum(&*data);
6060
success = !is_corrupted && !is_stale;
6161
if (is_corrupted) {
62-
delete data;
63-
data = nullptr;
62+
data = std::nullopt;
6463
}
6564
} else {
66-
delete data;
67-
data = nullptr;
65+
data = std::nullopt;
6866
}
6967
data_ready_(success, key, data);
7068
delete this;
@@ -83,10 +81,9 @@ ValidatingStorage::ValidatingStorage(Storage* storage)
8381

8482
ValidatingStorage::~ValidatingStorage() = default;
8583

86-
void ValidatingStorage::Put(const std::string& key, std::string* data) {
87-
assert(data != nullptr);
88-
ValidatingUtil::Wrap(std::time(nullptr), data);
89-
wrapped_storage_->Put(key, data);
84+
void ValidatingStorage::Put(const std::string& key, std::string data) {
85+
ValidatingUtil::Wrap(std::time(nullptr), &data);
86+
wrapped_storage_->Put(key, std::move(data));
9087
}
9188

9289
void ValidatingStorage::Get(const std::string& key,

‎cpp/src/validating_storage.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ class ValidatingStorage : public Storage {
4444
~ValidatingStorage() override;
4545

4646
// Storage implementation.
47-
void Put(const std::string& key, std::string* data) override;
47+
void Put(const std::string& key, std::string data) override;
4848

4949
// Storage implementation.
5050
// If the data is invalid, then |data_ready| will be called with (false, key,

‎cpp/test/fake_storage.cc‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,13 @@ namespace addressinput {
2323

2424
FakeStorage::FakeStorage() = default;
2525

26-
FakeStorage::~FakeStorage() {
27-
for (const auto& pair : data_) {
28-
delete pair.second;
29-
}
30-
}
26+
FakeStorage::~FakeStorage() = default;
3127

32-
void FakeStorage::Put(const std::string& key, std::string* data) {
33-
assert(data != nullptr);
34-
auto result = data_.emplace(key, data);
28+
void FakeStorage::Put(const std::string& key, std::string data) {
29+
auto result = data_.emplace(key, std::move(data));
3530
if (!result.second) {
3631
// Replace data in existing entry for this key.
37-
delete result.first->second;
38-
result.first->second = data;
32+
result.first->second = std::move(data);
3933
}
4034
}
4135

@@ -44,7 +38,7 @@ void FakeStorage::Get(const std::string& key,
4438
auto data_it = data_.find(key);
4539
bool success = data_it != data_.end();
4640
data_ready(success, key,
47-
success ? new std::string(*data_it->second) : nullptr);
41+
success ? std::make_optional<std::string>(data_it->second) : std::nullopt);
4842
}
4943

5044
} // namespace addressinput

‎cpp/test/fake_storage.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,11 @@ class FakeStorage : public Storage {
6565
~FakeStorage() override;
6666

6767
// Storage implementation.
68-
void Put(const std::string& key, std::string* data) override;
68+
void Put(const std::string& key, std::string data) override;
6969
void Get(const std::string& key, const Callback& data_ready) const override;
7070

7171
private:
72-
std::map<std::string, std::string*> data_;
72+
std::map<std::string, std::string> data_;
7373
};
7474

7575
} // namespace addressinput

‎cpp/test/fake_storage_test.cc‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,12 @@ class FakeStorageTest : public testing::Test {
5050
const std::unique_ptr<const Storage::Callback> data_ready_;
5151

5252
private:
53-
void OnDataReady(bool success, const std::string& key, std::string* data) {
54-
ASSERT_FALSE(success && data == nullptr);
53+
void OnDataReady(bool success, const std::string& key, std::optional<std::string> data) {
54+
ASSERT_FALSE(success && !data.has_value());
5555
success_ = success;
5656
key_ = key;
57-
if (data != nullptr) {
58-
data_ = *data;
59-
delete data;
57+
if (data.has_value()) {
58+
data_ = std::move(data).value();
6059
}
6160
}
6261
};
@@ -70,7 +69,7 @@ TEST_F(FakeStorageTest, GetWithoutPutReturnsEmptyData) {
7069
}
7170

7271
TEST_F(FakeStorageTest, GetReturnsWhatWasPut) {
73-
storage_.Put("key", new std::string("value"));
72+
storage_.Put("key", std::string("value"));
7473
storage_.Get("key", *data_ready_);
7574

7675
EXPECT_TRUE(success_);
@@ -79,8 +78,8 @@ TEST_F(FakeStorageTest, GetReturnsWhatWasPut) {
7978
}
8079

8180
TEST_F(FakeStorageTest, SecondPutOverwritesData) {
82-
storage_.Put("key", new std::string("bad-value"));
83-
storage_.Put("key", new std::string("good-value"));
81+
storage_.Put("key", std::string("bad-value"));
82+
storage_.Put("key", std::string("good-value"));
8483
storage_.Get("key", *data_ready_);
8584

8685
EXPECT_TRUE(success_);

0 commit comments

Comments
 (0)