Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1462763002:
Remove ScopedPtrMap from /cc (Closed)
DescriptionRemove ScopedPtrMap from /cc
C++ 11 enables containers that contain move-only type, scoped_ptr.
So, Use std::map<key, scoped_ptr<Foo>> instead of ScopedPtrMap.
BUG=554291
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/cdbfa816b2cae37df31701d0415bac84f414e616
Cr-Commit-Position: refs/heads/master@{#361315}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Matt's review #
Messages
Total messages: 25 (12 generated)
Description was changed from ========== Remove ScopedPtrMap from /cc BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Remove ScopedPtrMap from /cc C++ 11 enables containers that contain move-only type, scoped_ptr. So, Use std::map<key, scoped_ptr<Foo>> instead of ScopedPtrMap. BUG=554291 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
limasdf@gmail.com changed reviewers: + mgiuca@chromium.org
Dear Mr. Matt, Would you mind if I ask for your wisdom to solve this? https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:171: scoped_ptr<PoolResource> my_resource = it->second.Pass(); take_and_erase() returns scoped_ptr and erase iterator. I did same thing but doesn't work.
> Dear Mr. Matt, It's Dr. Matt :) https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:109: in_use_resources_[resource->id()] = it->Pass(); nit: It seems that std::move is allowed now. I'm not sure what our policy is on it, but it seems prudent to use it in any new or updated code. Could you try changing it->Pass() to std::move(it) and see if it works? https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:109: in_use_resources_[resource->id()] = it->Pass(); nit: There is a trailing space at the end of this line. https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:171: scoped_ptr<PoolResource> my_resource = it->second.Pass(); On 2015/11/19 13:48:51, limasdf wrote: > take_and_erase() returns scoped_ptr and erase iterator. > I did same thing but doesn't work. What do you mean it doesn't work? (I patched your CL in and it compiles fine.) I think what you've got here is the correct substitute for take_and_erase. Just delete that commented-out line. Also use std::move instead of Pass. take_and_erase is something we added specifically to ScopedPtrMap to get around a deficiency: we weren't really storing scoped_ptrs in the map so the client cannot simply std::move the object out of there. take_and_erase is the only way to move an object out of a ScopedPtrMap once you've put it in. For a std::map<scoped_ptr> you should just be able to std::move the object out of the iterator, then erase the iterator. https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:171: scoped_ptr<PoolResource> my_resource = it->second.Pass(); This code can be simplified. The above code gets a raw pointer using it->second.get(). You then immediately get a scoped_ptr to the same object. Just replace the line above with: scoped_ptr<PoolResource> pool_resource = std::move(it->second); Then you don't need a second variable my_resource.
Thank you, Dr. Matt! :) PTAL! https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:109: in_use_resources_[resource->id()] = it->Pass(); On 2015/11/19 23:42:26, Matt Giuca wrote: > nit: It seems that std::move is allowed now. I'm not sure what our policy is on > it, but it seems prudent to use it in any new or updated code. Could you try > changing it->Pass() to std::move(it) and see if it works? Done. https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:171: scoped_ptr<PoolResource> my_resource = it->second.Pass(); On 2015/11/19 23:42:26, Matt Giuca wrote: > On 2015/11/19 13:48:51, limasdf wrote: > > take_and_erase() returns scoped_ptr and erase iterator. > > I did same thing but doesn't work. > > What do you mean it doesn't work? (I patched your CL in and it compiles fine.) I > think what you've got here is the correct substitute for take_and_erase. Just > delete that commented-out line. > > Also use std::move instead of Pass. > > take_and_erase is something we added specifically to ScopedPtrMap to get around > a deficiency: we weren't really storing scoped_ptrs in the map so the client > cannot simply std::move the object out of there. take_and_erase is the only way > to move an object out of a ScopedPtrMap once you've put it in. > > For a std::map<scoped_ptr> you should just be able to std::move the object out > of the iterator, then erase the iterator. Sorry. Because of trybot fail, I was thinking there's some problem. but it was wrong result(flaky?). Trybot is happy now :) https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:171: scoped_ptr<PoolResource> my_resource = it->second.Pass(); On 2015/11/19 23:42:26, Matt Giuca wrote: > This code can be simplified. The above code gets a raw pointer using > it->second.get(). You then immediately get a scoped_ptr to the same object. > > Just replace the line above with: > scoped_ptr<PoolResource> pool_resource = std::move(it->second); > > Then you don't need a second variable my_resource. On line#175, |pool_resource| is being used, so keeping raw pointer. and passed it->second.
Thanks, limasdf! LGTM. (Though you will need an OWNER's approval as well.) And thanks for taking this initiative. I see you've done many ports over to std::map. https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/1462763002/diff/40001/cc/resources/resource_p... cc/resources/resource_pool.cc:171: scoped_ptr<PoolResource> my_resource = it->second.Pass(); On 2015/11/20 15:20:05, limasdf wrote: > On 2015/11/19 23:42:26, Matt Giuca wrote: > > This code can be simplified. The above code gets a raw pointer using > > it->second.get(). You then immediately get a scoped_ptr to the same object. > > > > Just replace the line above with: > > scoped_ptr<PoolResource> pool_resource = std::move(it->second); > > > > Then you don't need a second variable my_resource. > > On line#175, |pool_resource| is being used, so keeping raw pointer. and passed > it->second. Acknowledged.
limasdf@gmail.com changed reviewers: + danakj@chromium.org
limasdf@gmail.com changed reviewers: + enne@chromium.org - danakj@chromium.org
I am happy to do this! Adding enne, the owner of /cc
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
lgtm
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462763002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462763002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by limasdf@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1462763002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1462763002/60001
Message was sent while issue was closed.
Committed patchset #2 (id:60001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/cdbfa816b2cae37df31701d0415bac84f414e616 Cr-Commit-Position: refs/heads/master@{#361315} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
