Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1927753002:
Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base (Closed)
|
Created:
4 years, 7 months ago by dcheng Modified:
4 years, 7 months ago CC:
chromium-reviews, Robert Sesek, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionConvert callers of base::DeepCopy() to base::CreateDeepCopy() in //base
BUG=581865
Committed: https://crrev.com/e6d1c78bf4f6ec1d5aa451e9fc9e2979085eb915
Cr-Commit-Position: refs/heads/master@{#390450}
Patch Set 1 #
Total comments: 6
Patch Set 2 : . #Patch Set 3 : With a comment #
Total comments: 2
Patch Set 4 : . #
Total comments: 13
Messages
Total messages: 24 (6 generated)
dcheng@chromium.org changed reviewers: + danakj@chromium.org, jbroman@chromium.org, thakis@chromium.org
+danakj as the primary review, but adding C++ style owners as well for std::string vs std::unique_ptr<std::string>. https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) { How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any thoughts?
https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) { On 2016/04/27 at 22:12:11, dcheng wrote: > How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any thoughts? I prefer std::string to std::unique_ptr<std::string>, unless there's some reason to use unique_ptr which I don't see here. The string already knows about allocating a buffer, already supports moving, things are simpler that way, etc. https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... base/json/json_parser.cc:152: #if 0 remove before landing?
https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... base/json/json_parser.cc:152: #if 0 On 2016/04/27 at 22:19:39, jbroman wrote: > remove before landing? Should already be gone in the latest PS.
https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) { On 2016/04/27 at 22:19:39, jbroman wrote: > On 2016/04/27 at 22:12:11, dcheng wrote: > > How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any thoughts? > > I prefer std::string to std::unique_ptr<std::string>, unless there's some reason to use unique_ptr which I don't see here. > > The string already knows about allocating a buffer, already supports moving, things are simpler that way, etc. Having now read more of the surrounding code, I think you _do_ need std::unique_ptr<std::string> here if you want to be correct. This line: start_pos_ = input_copy->data(); Takes a pointer into the string buffer, which may become invalid if you move the std::string. In particular, the "small string" optimization can allocate the buffer inside the std::string, and so you'd get a different buffer after you moved (because the short string would be copied). Otherwise, you'll need to take the const char* after moving the string into the final Value, not before. That looks like it might be both inconvenient and subtle.
https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) { On 2016/04/27 at 22:25:24, jbroman wrote: > On 2016/04/27 at 22:19:39, jbroman wrote: > > On 2016/04/27 at 22:12:11, dcheng wrote: > > > How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any thoughts? > > > > I prefer std::string to std::unique_ptr<std::string>, unless there's some reason to use unique_ptr which I don't see here. > > > > The string already knows about allocating a buffer, already supports moving, things are simpler that way, etc. > > Having now read more of the surrounding code, I think you _do_ need std::unique_ptr<std::string> here if you want to be correct. This line: > > start_pos_ = input_copy->data(); > > Takes a pointer into the string buffer, which may become invalid if you move the std::string. In particular, the "small string" optimization can allocate the buffer inside the std::string, and so you'd get a different buffer after you moved (because the short string would be copied). > > Otherwise, you'll need to take the const char* after moving the string into the final Value, not before. That looks like it might be both inconvenient and subtle. I don't think start_pos_ is used after parsing. Even in the original code, there are instances where it'd end up pointing into a destroyed std::string.
On 2016/04/27 at 22:38:27, dcheng wrote: > https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc > File base/json/json_parser.cc (right): > > https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... > base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) { > On 2016/04/27 at 22:25:24, jbroman wrote: > > On 2016/04/27 at 22:19:39, jbroman wrote: > > > On 2016/04/27 at 22:12:11, dcheng wrote: > > > > How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any thoughts? > > > > > > I prefer std::string to std::unique_ptr<std::string>, unless there's some reason to use unique_ptr which I don't see here. > > > > > > The string already knows about allocating a buffer, already supports moving, things are simpler that way, etc. > > > > Having now read more of the surrounding code, I think you _do_ need std::unique_ptr<std::string> here if you want to be correct. This line: > > > > start_pos_ = input_copy->data(); > > > > Takes a pointer into the string buffer, which may become invalid if you move the std::string. In particular, the "small string" optimization can allocate the buffer inside the std::string, and so you'd get a different buffer after you moved (because the short string would be copied). > > > > Otherwise, you'll need to take the const char* after moving the string into the final Value, not before. That looks like it might be both inconvenient and subtle. > > I don't think start_pos_ is used after parsing. Even in the original code, there are instances where it'd end up pointing into a destroyed std::string. If those aren't used after the time the *HiddenRootValue are constructed, and nor is any other pointer into the string, then why do the *HiddenRootValue hold json_ at all? Nobody ever accesses it, according to Code Search, so I assumed it was there so that pointers could remain valid.
On 2016/04/27 at 22:51:16, jbroman wrote: > On 2016/04/27 at 22:38:27, dcheng wrote: > > https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc > > File base/json/json_parser.cc (right): > > > > https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... > > base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* root) : json_(std::move(json)) { > > On 2016/04/27 at 22:25:24, jbroman wrote: > > > On 2016/04/27 at 22:19:39, jbroman wrote: > > > > On 2016/04/27 at 22:12:11, dcheng wrote: > > > > > How do we feel about this? Or do we prefer std::unique_ptr<std::string>? Any thoughts? > > > > > > > > I prefer std::string to std::unique_ptr<std::string>, unless there's some reason to use unique_ptr which I don't see here. > > > > > > > > The string already knows about allocating a buffer, already supports moving, things are simpler that way, etc. > > > > > > Having now read more of the surrounding code, I think you _do_ need std::unique_ptr<std::string> here if you want to be correct. This line: > > > > > > start_pos_ = input_copy->data(); > > > > > > Takes a pointer into the string buffer, which may become invalid if you move the std::string. In particular, the "small string" optimization can allocate the buffer inside the std::string, and so you'd get a different buffer after you moved (because the short string would be copied). > > > > > > Otherwise, you'll need to take the const char* after moving the string into the final Value, not before. That looks like it might be both inconvenient and subtle. > > > > I don't think start_pos_ is used after parsing. Even in the original code, there are instances where it'd end up pointing into a destroyed std::string. > > If those aren't used after the time the *HiddenRootValue are constructed, and nor is any other pointer into the string, then why do the *HiddenRootValue hold json_ at all? Nobody ever accesses it, according to Code Search, so I assumed it was there so that pointers could remain valid. Hmm... good point. I don't know. Let me keep poking. Some unit tests are failing, which probably means something is wrong.
On 2016/04/27 22:55:30, dcheng wrote: > On 2016/04/27 at 22:51:16, jbroman wrote: > > On 2016/04/27 at 22:38:27, dcheng wrote: > > > https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc > > > File base/json/json_parser.cc (right): > > > > > > > https://codereview.chromium.org/1927753002/diff/1/base/json/json_parser.cc#ne... > > > base/json/json_parser.cc:89: ListHiddenRootValue(std::string json, Value* > root) : json_(std::move(json)) { > > > On 2016/04/27 at 22:25:24, jbroman wrote: > > > > On 2016/04/27 at 22:19:39, jbroman wrote: > > > > > On 2016/04/27 at 22:12:11, dcheng wrote: > > > > > > How do we feel about this? Or do we prefer > std::unique_ptr<std::string>? Any thoughts? > > > > > > > > > > I prefer std::string to std::unique_ptr<std::string>, unless there's > some reason to use unique_ptr which I don't see here. > > > > > > > > > > The string already knows about allocating a buffer, already supports > moving, things are simpler that way, etc. > > > > > > > > Having now read more of the surrounding code, I think you _do_ need > std::unique_ptr<std::string> here if you want to be correct. This line: > > > > > > > > start_pos_ = input_copy->data(); > > > > > > > > Takes a pointer into the string buffer, which may become invalid if you > move the std::string. In particular, the "small string" optimization can > allocate the buffer inside the std::string, and so you'd get a different buffer > after you moved (because the short string would be copied). > > > > > > > > Otherwise, you'll need to take the const char* after moving the string > into the final Value, not before. That looks like it might be both inconvenient > and subtle. > > > > > > I don't think start_pos_ is used after parsing. Even in the original code, > there are instances where it'd end up pointing into a destroyed std::string. > > > > If those aren't used after the time the *HiddenRootValue are constructed, and > nor is any other pointer into the string, then why do the *HiddenRootValue hold > json_ at all? Nobody ever accesses it, according to Code Search, so I assumed it > was there so that pointers could remain valid. > > Hmm... good point. > > I don't know. Let me keep poking. Some unit tests are failing, which probably > means something is wrong. I prefer std::string over unique_ptr<std::string> too if it works.
PTAL. I reverted it to std::unique_ptr<std::string> and added some comments to more explicitly describe how the optimization works.
Description was changed from ========== Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base BUG=581865 ========== to ========== Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base BUG=581865 ==========
non-owner lgtm https://codereview.chromium.org/1927753002/diff/40001/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/40001/base/json/json_parser.c... base/json/json_parser.cc:39: DictionaryHiddenRootValue(std::unique_ptr<std::string> json, Value* root) nit: you might also consider making the second argument std::unique_ptr<Value> while you're here, since its ownership is essentially transferred. It's not a big difference, but it might make it slightly more obvious at the call site that it's destructive to the second argument.
https://codereview.chromium.org/1927753002/diff/40001/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/40001/base/json/json_parser.c... base/json/json_parser.cc:39: DictionaryHiddenRootValue(std::unique_ptr<std::string> json, Value* root) On 2016/04/28 at 16:46:33, jbroman wrote: > nit: you might also consider making the second argument std::unique_ptr<Value> while you're here, since its ownership is essentially transferred. It's not a big difference, but it might make it slightly more obvious at the call site that it's destructive to the second argument. Done.
LGTM https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:43: DictionaryValue::Swap(static_cast<DictionaryValue*>(root.get())); This should be passing the unique_ptr. Can you TODO somewhere? https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:95: ListValue::Swap(static_cast<ListValue*>(root.get())); ditto https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:123: if (!ListValue::Remove(index, &out_owned)) heh, we should just have this return std::unique_ptr and null means the same as false currently https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:143: : Value(TYPE_STRING), string_piece_(piece) {} nit: move piece? (i know its the same as copy atm) https://codereview.chromium.org/1927753002/diff/60001/base/json/json_reader.cc File base/json/json_reader.cc (right): https://codereview.chromium.org/1927753002/diff/60001/base/json/json_reader.c... base/json/json_reader.cc:48: return parser.Parse(json); nit: move all the StringPieces?
https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:43: DictionaryValue::Swap(static_cast<DictionaryValue*>(root.get())); On 2016/04/28 at 17:39:37, danakj wrote: > This should be passing the unique_ptr. Can you TODO somewhere? Should it though? It's not really moving/taking ownership of it so much as swapping the contents: after all, things like std::vector::swap() don't require passing in an rvalue reference. https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:95: ListValue::Swap(static_cast<ListValue*>(root.get())); On 2016/04/28 at 17:39:37, danakj wrote: > ditto See previous. https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:123: if (!ListValue::Remove(index, &out_owned)) On 2016/04/28 at 17:39:37, danakj wrote: > heh, we should just have this return std::unique_ptr and null means the same as false currently Acknowledged. https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:143: : Value(TYPE_STRING), string_piece_(piece) {} On 2016/04/28 at 17:39:37, danakj wrote: > nit: move piece? (i know its the same as copy atm) Why though? copy on a StringPiece will be cheaper than move anyway. https://codereview.chromium.org/1927753002/diff/60001/base/json/json_reader.cc File base/json/json_reader.cc (right): https://codereview.chromium.org/1927753002/diff/60001/base/json/json_reader.c... base/json/json_reader.cc:48: return parser.Parse(json); On 2016/04/28 at 17:39:37, danakj wrote: > nit: move all the StringPieces? See previous.
https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:143: : Value(TYPE_STRING), string_piece_(piece) {} On 2016/04/28 at 17:39:37, danakj wrote: > nit: move piece? (i know its the same as copy atm) nit-nit: base::StringPiece explicitly promises in its header that it's essentially free to copy and consists only of a pointer + length. Moving it feel like writing using std::move on an int, gfx::Point, or similar.
https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.cc File base/json/json_parser.cc (right): https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:43: DictionaryValue::Swap(static_cast<DictionaryValue*>(root.get())); On 2016/04/28 18:03:23, dcheng wrote: > On 2016/04/28 at 17:39:37, danakj wrote: > > This should be passing the unique_ptr. Can you TODO somewhere? > > Should it though? It's not really moving/taking ownership of it so much as > swapping the contents: after all, things like std::vector::swap() don't require > passing in an rvalue reference. Oh I guess you're right yah, it's swapping stuff into this Value here. ok. https://codereview.chromium.org/1927753002/diff/60001/base/json/json_parser.c... base/json/json_parser.cc:143: : Value(TYPE_STRING), string_piece_(piece) {} On 2016/04/28 18:06:18, jbroman wrote: > On 2016/04/28 at 17:39:37, danakj wrote: > > nit: move piece? (i know its the same as copy atm) > > nit-nit: base::StringPiece explicitly promises in its header that it's > essentially free to copy and consists only of a pointer + length. Moving it feel > like writing using std::move on an int, gfx::Point, or similar. Ah I see I see. #newtostrings. LGTM
The CQ bit was checked by dcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/1927753002/#ps60001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1927753002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1927753002/60001
Message was sent while issue was closed.
Description was changed from ========== Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base BUG=581865 ========== to ========== Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base BUG=581865 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e6d1c78bf4f6ec1d5aa451e9fc9e2979085eb915 Cr-Commit-Position: refs/heads/master@{#390450}
Message was sent while issue was closed.
Description was changed from ========== Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base BUG=581865 ========== to ========== Convert callers of base::DeepCopy() to base::CreateDeepCopy() in //base BUG=581865 Committed: https://crrev.com/e6d1c78bf4f6ec1d5aa451e9fc9e2979085eb915 Cr-Commit-Position: refs/heads/master@{#390450} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
