Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138103002:
base: Use scoped_ptr for ownership of pointers in unittests. (Closed)
Descriptionbase: Use scoped_ptr for ownership of pointers in unittests.
This puts all heap allocated values into scoped_ptrs in
value_unittest.cc and json_writer_unittest.cc. This way
ownership is explicit in the code and we don't use raw
pointers to change owners of an object.
R=thakis@chromium.org
Committed: https://crrev.com/8dba5a567ca630e38e20a6c59c5827ad481b105d
Cr-Commit-Position: refs/heads/master@{#329235}
Patch Set 1 #Patch Set 2 : scopedptrtests: . #
Total comments: 2
Patch Set 3 : scopedptrtests: . #
Total comments: 7
Patch Set 4 : scopedptrtests: stack #
Total comments: 1
Patch Set 5 : scopedptrtests: heretoo #
Messages
Total messages: 17 (3 generated)
nice! https://codereview.chromium.org/1138103002/diff/20001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/20001/base/json/json_writer_u... base/json/json_writer_unittest.cc:15: scoped_ptr<Value> root = make_scoped_ptr(Value::CreateNullValue()); Do you need make_scoped_ptr here? Doesn't just scoped_ptr<Value> root(Value::CreateNullValue()); work? (I thought make_scoped_ptr is when you call a function that takes a scoped_ptr so that you don't have to write scoped_ptr<Type>(foo()) when foo() already returns a Type* – but you're already typing out Type on the lhs here so make_scoped_ptr doesn't do much here?)
https://codereview.chromium.org/1138103002/diff/20001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/20001/base/json/json_writer_u... base/json/json_writer_unittest.cc:15: scoped_ptr<Value> root = make_scoped_ptr(Value::CreateNullValue()); On 2015/05/11 18:16:08, Nico wrote: > Do you need make_scoped_ptr here? Doesn't just > > scoped_ptr<Value> root(Value::CreateNullValue()); > > work? (I thought make_scoped_ptr is when you call a function that takes a > scoped_ptr so that you don't have to write scoped_ptr<Type>(foo()) when foo() > already returns a Type* – but you're already typing out Type on the lhs here so > make_scoped_ptr doesn't do much here?) Ya I can/should use that constructor instead, done. I was using the scoped_ptr<U>&& constructor.
estade@chromium.org changed reviewers: + estade@chromium.org
https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:16: EXPECT_TRUE(JSONWriter::Write(root.get(), &output_js)); EXPECT_TRUE(JSONWriter::Write(Value::CreateNullValue().get(), &output_js)); (done here: https://codereview.chromium.org/1129083003/diff/60001/base/json/json_writer_u... ) https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new DictionaryValue); from here down, these should all be stack allocated. I've done this here: https://codereview.chromium.org/1131113004/
https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:16: EXPECT_TRUE(JSONWriter::Write(root.get(), &output_js)); On 2015/05/11 18:27:20, Evan Stade wrote: > EXPECT_TRUE(JSONWriter::Write(Value::CreateNullValue().get(), &output_js)); This only works if CreateNullValue() returns a scoped_ptr which it does not yet? > (done here: > https://codereview.chromium.org/1129083003/diff/60001/base/json/json_writer_u... > ) https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new DictionaryValue); On 2015/05/11 18:27:20, Evan Stade wrote: > from here down, these should all be stack allocated. I've done this here: > https://codereview.chromium.org/1131113004/ Done.
https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new DictionaryValue); On 2015/05/11 18:31:44, danakj wrote: > On 2015/05/11 18:27:20, Evan Stade wrote: > > from here down, these should all be stack allocated. I've done this here: > > https://codereview.chromium.org/1131113004/ > > Done. The implication I intended from both of these comments was that you might as well leave this test alone as it's already getting fixed.
https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new DictionaryValue); On 2015/05/11 18:49:36, Evan Stade wrote: > On 2015/05/11 18:31:44, danakj wrote: > > On 2015/05/11 18:27:20, Evan Stade wrote: > > > from here down, these should all be stack allocated. I've done this here: > > > https://codereview.chromium.org/1131113004/ > > > > Done. > > The implication I intended from both of these comments was that you might as > well leave this test alone as it's already getting fixed. I see, I can go back to the previous patch set if you like, I just prefer not to make us use scoped_ptr everywhere else but not here. Your patch can change the CreateNullValue to not use a local variable if you like.
On 2015/05/11 18:51:34, danakj wrote: > https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... > File base/json/json_writer_unittest.cc (right): > > https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... > base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new > DictionaryValue); > On 2015/05/11 18:49:36, Evan Stade wrote: > > On 2015/05/11 18:31:44, danakj wrote: > > > On 2015/05/11 18:27:20, Evan Stade wrote: > > > > from here down, these should all be stack allocated. I've done this here: > > > > https://codereview.chromium.org/1131113004/ > > > > > > Done. > > > > The implication I intended from both of these comments was that you might as > > well leave this test alone as it's already getting fixed. > > I see, I can go back to the previous patch set if you like, I just prefer not to > make us use scoped_ptr everywhere else but not here. Your patch can change the > CreateNullValue to not use a local variable if you like. I agree with the correctness of using scoped_ptr. I don't understand the urgency to fix this now instead of 2 days from now.
On 2015/05/11 18:53:11, Evan Stade wrote: > On 2015/05/11 18:51:34, danakj wrote: > > > https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... > > File base/json/json_writer_unittest.cc (right): > > > > > https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... > > base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new > > DictionaryValue); > > On 2015/05/11 18:49:36, Evan Stade wrote: > > > On 2015/05/11 18:31:44, danakj wrote: > > > > On 2015/05/11 18:27:20, Evan Stade wrote: > > > > > from here down, these should all be stack allocated. I've done this > here: > > > > > https://codereview.chromium.org/1131113004/ > > > > > > > > Done. > > > > > > The implication I intended from both of these comments was that you might as > > > well leave this test alone as it's already getting fixed. > > > > I see, I can go back to the previous patch set if you like, I just prefer not > to > > make us use scoped_ptr everywhere else but not here. Your patch can change the > > CreateNullValue to not use a local variable if you like. > > I agree with the correctness of using scoped_ptr. I don't understand the urgency > to fix this now instead of 2 days from now. I wrote this CL just so that I could support your change for base::Value since everything will be scoped_ptrs in the tests instead of just some things. Then we can iterate from that state by changing Value APIs to enforce scoped_ptr usage. What I dislike is a patch taking us toward using scoped_ptr but only going part way when it's just a few small changes to use it entirely. Splitting that across multiple CLs leaves us much more likely to forget something (maybe a piece gets reverted, or reviewing requires more attention) and doesn't give a complete step with each individual CL. So I think it's a negative to change this CL to leave some things not as scoped_ptrs.
On 2015/05/11 19:11:53, danakj wrote: > On 2015/05/11 18:53:11, Evan Stade wrote: > > On 2015/05/11 18:51:34, danakj wrote: > > > > > > https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... > > > File base/json/json_writer_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... > > > base/json/json_writer_unittest.cc:20: root = make_scoped_ptr(new > > > DictionaryValue); > > > On 2015/05/11 18:49:36, Evan Stade wrote: > > > > On 2015/05/11 18:31:44, danakj wrote: > > > > > On 2015/05/11 18:27:20, Evan Stade wrote: > > > > > > from here down, these should all be stack allocated. I've done this > > here: > > > > > > https://codereview.chromium.org/1131113004/ > > > > > > > > > > Done. > > > > > > > > The implication I intended from both of these comments was that you might > as > > > > well leave this test alone as it's already getting fixed. > > > > > > I see, I can go back to the previous patch set if you like, I just prefer > not > > to > > > make us use scoped_ptr everywhere else but not here. Your patch can change > the > > > CreateNullValue to not use a local variable if you like. > > > > I agree with the correctness of using scoped_ptr. I don't understand the > urgency > > to fix this now instead of 2 days from now. > > I wrote this CL just so that I could support your change for base::Value since > everything will be scoped_ptrs in the tests instead of just some things. I was under the impression that you recused yourself of reviewing that CL? > Then we > can iterate from that state by changing Value APIs to enforce scoped_ptr usage. > > What I dislike is a patch taking us toward using scoped_ptr but only going part > way when it's just a few small changes to use it entirely. Splitting that across > multiple CLs leaves us much more likely to forget something (maybe a piece gets > reverted, or reviewing requires more attention) and doesn't give a complete step > with each individual CL. So I think it's a negative to change this CL to leave > some things not as scoped_ptrs. We're a long way off from getting rid of the bad old APIs in Value. It's not a few small changes. There's a lot of stuff outside of base/ that would need to be updated first. When that day comes, minor odds and ends we missed would be flushed out. That said, the main reason I'm miffed is that you are touching code for which there are already changes in flight. I think that is not the best use of time.
> We're a long way off from getting rid of the bad old APIs in Value. It's not a > few small changes. There's a lot of stuff outside of base/ that would need to be > updated first. When that day comes, minor odds and ends we missed would be > flushed out. That said, the main reason I'm miffed is that you are touching code > for which there are already changes in flight. I think that is not the best use > of time. Arguing about this probably isn't either :-), rebasing would take < 3 min I'd gues given how tiny this file iss, no? From what I understand, the argument for the changes here is that they'll make your change safer in some way. lgtm, I'd rather have too many CLs to clean things like this up even if there's a small overlap every now and then (and in this case it's small enough that I feel rebasing doesn't cause a lot of pain). (I somehow managed to leave the same comment twice below. It's enough if you address it once :-P) https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/40001/base/json/json_writer_u... base/json/json_writer_unittest.cc:123: make_scoped_ptr(BinaryValue::CreateWithCopiedBuffer("asdf", 4)); here too https://codereview.chromium.org/1138103002/diff/60001/base/json/json_writer_u... File base/json/json_writer_unittest.cc (right): https://codereview.chromium.org/1138103002/diff/60001/base/json/json_writer_u... base/json/json_writer_unittest.cc:123: make_scoped_ptr(BinaryValue::CreateWithCopiedBuffer("asdf", 4)); no make_scoped_ptr here either
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/1138103002/#ps80001 (title: "scopedptrtests: heretoo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138103002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8dba5a567ca630e38e20a6c59c5827ad481b105d Cr-Commit-Position: refs/heads/master@{#329235} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
