Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2262423002:
Use a vector of smart pointers for callback return type. (Closed)
|
Created:
4 years, 4 months ago by Pete Williamson Modified:
4 years, 3 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse a vector of smart pointers for callback return type.
We can't use references to large objects with std::move,
so change the code to use the vector of pointers itself
instead of a reference to a vector of pointers.
BUG=637077
Committed: https://crrev.com/2f65c71a4b658c5555806222dda7fd3a1e39c31c
Cr-Commit-Position: refs/heads/master@{#417462}
Patch Set 1 #Patch Set 2 : Merge with tip of tree #Patch Set 3 : Logical merge fixes #Patch Set 4 : Fix build problems #Patch Set 5 : Logical merge fixes #
Total comments: 4
Patch Set 6 : CR fixes per Dimich #Patch Set 7 : Merge fixup #Patch Set 8 : More compile fixes #
Total comments: 2
Patch Set 9 : still more merging #Patch Set 10 : another merge. #
Total comments: 6
Patch Set 11 : for -> for_each #Patch Set 12 : CR feedback per BauerB #
Total comments: 15
Patch Set 13 : More CR feedback per BauerB #Patch Set 14 : Fix compile and test #Patch Set 15 : Merge #
Total comments: 5
Messages
Total messages: 82 (55 generated)
petewil@chromium.org changed reviewers: + dimich@chromium.org
As you requested, making sure we only make a single copy of each SavePageRequest as it flows through the callbacks and PostTask operations, by using std::move and base::Passed.
Description was changed from ========== Use a vector of smart pointers for callback return type. We can't use references to large objects with std::move, so change the code to use the vector of pointers itself instead of a reference to a vector of pointers. BUG=610521 ========== to ========== Use a vector of smart pointers for callback return type. We can't use references to large objects with std::move, so change the code to use the vector of pointers itself instead of a reference to a vector of pointers. BUG=637077 ==========
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Nice. I think it improves readability as well and std::unique_ptr is very explicit. https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.cc:211: std::vector<std::unique_ptr<SavePageRequest>>::iterator request; would something like this work: for (const auto& request : requests) NotifyChanged(request->get()); This would be more readable than iterator... https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:93: // copy the result right away, it goes out of scope at the end of the This comment needs updating.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.cc:211: std::vector<std::unique_ptr<SavePageRequest>>::iterator request; On 2016/08/26 23:39:20, Dmitry Titov wrote: > would something like this work: > for (const auto& request : requests) > NotifyChanged(request->get()); > > This would be more readable than iterator... Done. https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2262423002/diff/80001/components/offline_page... components/offline_pages/background/request_coordinator.h:93: // copy the result right away, it goes out of scope at the end of the On 2016/08/26 23:39:20, Dmitry Titov wrote: > This comment needs updating. Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
petewil@chromium.org changed reviewers: + dpapad@chromium.org
dpapad: PTAL at offline_internals_ui_message_handler.cc/h Thanks! Base idea of this change is to mininmize copying structs around during callbacks by moving data instead of copying it. Since we used the data structure in lots of places, the changelist touches lots of files.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by for style. https://codereview.chromium.org/2262423002/diff/140001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2262423002/diff/140001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:413: base::Passed(std::vector<std::unique_ptr<SavePageRequest>>())))) nit: because some of conditions are growing, you should be wrapping the following one-liners in {} applies in multiple places.
https://codereview.chromium.org/2262423002/diff/140001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2262423002/diff/140001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:413: base::Passed(std::vector<std::unique_ptr<SavePageRequest>>())))) On 2016/08/31 23:27:23, fgorski wrote: > nit: because some of conditions are growing, you should be wrapping the > following one-liners in {} > > applies in multiple places. Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
petewil@chromium.org changed reviewers: + bauerb@google.com
Adding bauerb (perhaps dpapad is out of office, bauerb seems to be returning). BauerB: Please take a look at offline_internals_ui_message_handler.cc/h Thanks! Base idea of this change is to mininmize copying structs around during callbacks by moving data instead of copying it. Since we used the data structure in lots of places, the changelist touches lots of files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bauerb@chromium.org changed reviewers: + bauerb@chromium.org - bauerb@google.com
Hm, in principle should be able to move vectors of objects as well, no? https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:325: for (request = requests.begin(); request != requests.end(); ++request) Why this style of loop? https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.h:74: std::vector<std::unique_ptr<SavePageRequest>>& requests, There are a lot of places here (in existing code as well) that use non-const references -- any reason why?
On 2016/09/07 10:13:41, Bernhard Bauer wrote: > Hm, in principle should be able to move vectors of objects as well, no? Yes, and this provides an impressive example: https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml?cl=head#copy... However it appears to be a recently-popular pattern in Chrome to explicitly use std::unique_ptr and std::move when ownership of objects is transferred. https://www.chromium.org/developers/smart-pointer-guidelines seem to imply that std::container<std_unique_ptr<Foo>> is a good way to go. If there is a doc on prescribed practice that we'd missed, please let us know!
Thanks for the review! Answers below. https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:325: for (request = requests.begin(); request != requests.end(); ++request) On 2016/09/07 10:13:41, Bernhard Bauer wrote: > Why this style of loop? I was having problems getting the type right for a for_each style loop. Fixed now. https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.h:74: std::vector<std::unique_ptr<SavePageRequest>>& requests, On 2016/09/07 10:13:41, Bernhard Bauer wrote: > There are a lot of places here (in existing code as well) that use non-const > references -- any reason why? const references won't work with moving (since you are destroying something in the place that you are moving it from, it can't be const). In this particular function, we are splitting requests from a vector of requests into two outparams. In just a few places, we are getting a const reference to this vector of smart pointers, because it is occasionally used without being moved or copied, but in most places it gets std::move-d to its final destination, so in most cases, we use non-const vectors to work with std::move()
petewil@chromium.org changed reviewers: - dpapad@chromium.org
-dpapad
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/07 16:18:00, Dmitry Titov wrote: > On 2016/09/07 10:13:41, Bernhard Bauer wrote: > > Hm, in principle should be able to move vectors of objects as well, no? > > Yes, and this provides an impressive example: > https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml?cl=head#copy... > However it appears to be a recently-popular pattern in Chrome to explicitly use > std::unique_ptr and std::move when ownership of objects is transferred. > https://www.chromium.org/developers/smart-pointer-guidelines seem to imply that > std::container<std_unique_ptr<Foo>> is a good way to go. > > If there is a doc on prescribed practice that we'd missed, please let us know! I think both are okay in Chromium, and I've definitely seen both. https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.h:74: std::vector<std::unique_ptr<SavePageRequest>>& requests, On 2016/09/07 20:21:01, Pete Williamson wrote: > On 2016/09/07 10:13:41, Bernhard Bauer wrote: > > There are a lot of places here (in existing code as well) that use non-const > > references -- any reason why? > > const references won't work with moving (since you are destroying something in > the place that you are moving it from, it can't be const). > > In this particular function, we are splitting requests from a vector of requests > into two outparams. OK, but outparams should be passed as pointers, no? And regular params that are moved should take the plain type. Right now you're using mutable references for both, which is not only discouraged by the style guide, but also makes the distinction between types of parameters less visible. > In just a few places, we are getting a const reference to this vector of smart > pointers, because it is occasionally used without being moved or copied, but in > most places it gets std::move-d to its final destination, so in most cases, we > use non-const vectors to work with std::move()
https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... File components/offline_pages/background/request_picker.h (right): https://codereview.chromium.org/2262423002/diff/180001/components/offline_pag... components/offline_pages/background/request_picker.h:74: std::vector<std::unique_ptr<SavePageRequest>>& requests, On 2016/09/07 21:43:52, Bernhard Bauer wrote: > On 2016/09/07 20:21:01, Pete Williamson wrote: > > On 2016/09/07 10:13:41, Bernhard Bauer wrote: > > > There are a lot of places here (in existing code as well) that use non-const > > > references -- any reason why? > > > > const references won't work with moving (since you are destroying something in > > the place that you are moving it from, it can't be const). > > > > In this particular function, we are splitting requests from a vector of > requests > > into two outparams. > > OK, but outparams should be passed as pointers, no? And regular params that are > moved should take the plain type. Right now you're using mutable references for > both, which is not only discouraged by the style guide, but also makes the > distinction between types of parameters less visible. > > > In just a few places, we are getting a const reference to this vector of smart > > pointers, because it is occasionally used without being moved or copied, but > in > > most places it gets std::move-d to its final destination, so in most cases, we > > use non-const vectors to work with std::move() > Done.
https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:325: for (const auto & request : requests) No space before &. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_picker.cc:66: std::vector<std::unique_ptr<SavePageRequest>>::iterator request; Here you could also use the C++-11-style loop. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_picker.cc:252: if (base::Time::Now() - request->get()->creation_time() >= And here as well (which would also remove one level of dereferencing.) https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:152: std::vector<std::unique_ptr<SavePageRequest>>& requests) { Pointer. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:186: std::vector<std::unique_ptr<SavePageRequest>>& requests) { Pointer. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:309: base::Passed(std::move(requests)))); FYI, you can also use base::Passed(&requests), which is a shortcut for this. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:243: ASSERT_TRUE(request == *(this->last_requests()[0].get())); I *think* the .get() is unnecessary -- you get the raw pointer out of a unique_ptr and then dereference it, right? If so, you can just directly dereference the unique_ptr.
https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_coordinator.cc:325: for (const auto & request : requests) On 2016/09/08 09:01:57, Bernhard Bauer wrote: > No space before &. Done. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_picker.cc:66: std::vector<std::unique_ptr<SavePageRequest>>::iterator request; On 2016/09/08 09:01:57, Bernhard Bauer wrote: > Here you could also use the C++-11-style loop. Done. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_picker.cc:252: if (base::Time::Now() - request->get()->creation_time() >= On 2016/09/08 09:01:57, Bernhard Bauer wrote: > And here as well (which would also remove one level of dereferencing.) Done. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:152: std::vector<std::unique_ptr<SavePageRequest>>& requests) { On 2016/09/08 09:01:57, Bernhard Bauer wrote: > Pointer. Done. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:186: std::vector<std::unique_ptr<SavePageRequest>>& requests) { On 2016/09/08 09:01:57, Bernhard Bauer wrote: > Pointer. If it's OK, I'd like to leave this for now - the ChangeRequestsState function will be removed by https://codereview.chromium.org/2249743005, and I'd like to reduce the merge pain. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:309: base::Passed(std::move(requests)))); On 2016/09/08 09:01:57, Bernhard Bauer wrote: > FYI, you can also use base::Passed(&requests), which is a shortcut for this. Done everywhere in this file. https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_queue_store_unittest.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_unittest.cc:243: ASSERT_TRUE(request == *(this->last_requests()[0].get())); On 2016/09/08 09:01:58, Bernhard Bauer wrote: > I *think* the .get() is unnecessary -- you get the raw pointer out of a > unique_ptr and then dereference it, right? If so, you can just directly > dereference the unique_ptr. Done.
Oops, this is not quite ready yet - I tested the wrong compile, have to fix a quick compile error and test error - new patch soon.
OK, fixed now, OK to review.
LGTM! https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... File components/offline_pages/background/request_queue_store_sql.cc (right): https://codereview.chromium.org/2262423002/diff/220001/components/offline_pag... components/offline_pages/background/request_queue_store_sql.cc:186: std::vector<std::unique_ptr<SavePageRequest>>& requests) { On 2016/09/08 17:27:00, Pete Williamson wrote: > On 2016/09/08 09:01:57, Bernhard Bauer wrote: > > Pointer. > > If it's OK, I'd like to leave this for now - the ChangeRequestsState function > will be removed by https://codereview.chromium.org/2249743005, and I'd like to > reduce the merge pain. OK.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2262423002/#ps260001 (title: "Fix compile and test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2262423002/#ps280001 (title: "Merge")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use a vector of smart pointers for callback return type. We can't use references to large objects with std::move, so change the code to use the vector of pointers itself instead of a reference to a vector of pointers. BUG=637077 ========== to ========== Use a vector of smart pointers for callback return type. We can't use references to large objects with std::move, so change the code to use the vector of pointers itself instead of a reference to a vector of pointers. BUG=637077 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001)
Message was sent while issue was closed.
Description was changed from ========== Use a vector of smart pointers for callback return type. We can't use references to large objects with std::move, so change the code to use the vector of pointers itself instead of a reference to a vector of pointers. BUG=637077 ========== to ========== Use a vector of smart pointers for callback return type. We can't use references to large objects with std::move, so change the code to use the vector of pointers itself instead of a reference to a vector of pointers. BUG=637077 Committed: https://crrev.com/2f65c71a4b658c5555806222dda7fd3a1e39c31c Cr-Commit-Position: refs/heads/master@{#417462} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/2f65c71a4b658c5555806222dda7fd3a1e39c31c Cr-Commit-Position: refs/heads/master@{#417462}
Message was sent while issue was closed.
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... components/offline_pages/background/request_picker.cc:243: std::vector<std::unique_ptr<SavePageRequest>>::iterator request; This local variable is not used and should be deleted. It currently triggers variable shadowing warnings on the experimental VC++ /analyze (static code analysis) build. https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... components/offline_pages/background/request_picker.cc:260: for (request = requests.begin(); request != requests.end(); ++request) I'm confused as to why this was changed to an explicit for loop - the inconsistency with the previous function seems odd, and I'm failing to see the reason. What am I missing?
Message was sent while issue was closed.
https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... components/offline_pages/background/request_picker.cc:243: std::vector<std::unique_ptr<SavePageRequest>>::iterator request; On 2016/09/12 17:20:04, brucedawson wrote: > This local variable is not used and should be deleted. It currently triggers > variable shadowing warnings on the experimental VC++ /analyze (static code > analysis) build. Removed in https://codereview.chromium.org/2332923003. https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... components/offline_pages/background/request_picker.cc:260: for (request = requests.begin(); request != requests.end(); ++request) On 2016/09/12 17:20:06, brucedawson wrote: > I'm confused as to why this was changed to an explicit for loop - the > inconsistency with the previous function seems odd, and I'm failing to see the > reason. What am I missing? Due to CR feedback, I changed some places to use the for_each style, but forgot to check the file for all other uses. Fixed in https://codereview.chromium.org/2332923003.
Message was sent while issue was closed.
https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... File components/offline_pages/background/request_picker.cc (right): https://codereview.chromium.org/2262423002/diff/280001/components/offline_pag... components/offline_pages/background/request_picker.cc:260: for (request = requests.begin(); request != requests.end(); ++request) On 2016/09/12 20:25:57, Pete Williamson wrote: > On 2016/09/12 17:20:06, brucedawson wrote: > > I'm confused as to why this was changed to an explicit for loop - the > > inconsistency with the previous function seems odd, and I'm failing to see the > > reason. What am I missing? > > Due to CR feedback, I changed some places to use the for_each style, but forgot > to check the file for all other uses. Fixed in > https://codereview.chromium.org/2332923003. Although, this one *started out* using for-each style, so that's curious. But, no matter. |
