Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2386883002:
Remove stl_util's deletion functions from net/spdy/. (Closed)
|
Created:
4 years, 2 months ago by Avi (use Gerrit) Modified:
4 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove stl_util's deletion functions from net/spdy/.
BUG=555865
Committed: https://crrev.com/ae73dbcef87d02504f6b3092a30bc6488ffd155e
Cr-Commit-Position: refs/heads/master@{#422495}
Patch Set 1 #Patch Set 2 : fit #Patch Set 3 : missing include #
Total comments: 16
Patch Set 4 : fixes #Patch Set 5 : clear #
Total comments: 4
Patch Set 6 : nits #
Messages
Total messages: 37 (27 generated)
The CQ bit was checked by avi@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-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by avi@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by avi@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.
The CQ bit was checked by avi@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.
avi@chromium.org changed reviewers: + davidben@chromium.org
davidben@chromium.org changed reviewers: + bnc@chromium.org
+bnc is probably a better reviewer for this code. (Does it have any relationship with the internal repository? I know QUIC does.) https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... File net/spdy/spdy_read_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:7: #include <algorithm> s/algorithm/utility/? https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:53: void SpdyReadQueue::Clear() { queue_.clear(); +bnc: It's distressing that no test noticed this. Would you mind adding one? https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.cc (left): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:146: base::STLDeleteElements(&erased_buffer_producers); // Invokes callbacks. This comment probably wants to be transplanted up to erased_buffer_producers. I assume it was done this way so all the callbacks are run after the queue is in some consistent state. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:162: base::STLDeleteElements(&erased_buffer_producers); // Invokes callbacks. Ditto. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:18: SpdyWriteQueue::PendingWrite::PendingWrite() : frame_producer(nullptr) {} If it's a unique_ptr, it doesn't need to be explicitly-initialized, right? Although, actually, is this constructor even needed? It seems to be more remnants of C++03 stuff. (Plus it leaves some fields uninitialized.) https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:55: frame_type, std::move(frame_producer), stream)); #include <utility> https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.h (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.h:84: std::deque<std::unique_ptr<PendingWrite>> queue_[NUM_PRIORITIES]; bnc: This is adding a layer of allocations here. Is this fine for this code? avi: Does it not work to write std::deque<PendingWrite> and give PendingWrite an PendingWrite&& constructor?
https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... File net/spdy/spdy_read_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:7: #include <algorithm> On 2016/10/03 16:16:31, davidben wrote: > s/algorithm/utility/? This is for the use of std::min, and http://en.cppreference.com/w/cpp/algorithm/min says it's <algorithm>. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:53: void SpdyReadQueue::Clear() { On 2016/10/03 16:16:31, davidben wrote: > queue_.clear(); > > +bnc: It's distressing that no test noticed this. Would you mind adding one? Is the request to add the clear call for me, or bnc@? I would prefer if this remained a refactoring-only CL. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:18: SpdyWriteQueue::PendingWrite::PendingWrite() : frame_producer(nullptr) {} On 2016/10/03 16:16:31, davidben wrote: > If it's a unique_ptr, it doesn't need to be explicitly-initialized, right? > > Although, actually, is this constructor even needed? It seems to be more > remnants of C++03 stuff. (Plus it leaves some fields uninitialized.) Done. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:55: frame_type, std::move(frame_producer), stream)); On 2016/10/03 16:16:31, davidben wrote: > #include <utility> Done. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.h (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.h:84: std::deque<std::unique_ptr<PendingWrite>> queue_[NUM_PRIORITIES]; On 2016/10/03 16:16:31, davidben wrote: > bnc: This is adding a layer of allocations here. Is this fine for this code? > > avi: Does it not work to write std::deque<PendingWrite> and give PendingWrite an > PendingWrite&& constructor? I'm looking at this again, and I don't know why I did it this way. Lemme try again.
The CQ bit was checked by avi@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... File net/spdy/spdy_read_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:7: #include <algorithm> On 2016/10/03 16:58:47, Avi wrote: > On 2016/10/03 16:16:31, davidben wrote: > > s/algorithm/utility/? > > This is for the use of std::min, and > http://en.cppreference.com/w/cpp/algorithm/min says it's <algorithm>. Ah, my bad! https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:53: void SpdyReadQueue::Clear() { On 2016/10/03 16:58:47, Avi wrote: > On 2016/10/03 16:16:31, davidben wrote: > > queue_.clear(); > > > > +bnc: It's distressing that no test noticed this. Would you mind adding one? > > Is the request to add the clear call for me, or bnc@? I would prefer if this > remained a refactoring-only CL. It's a request to add the clear() call plus a request for bnc@ to (in a separate CL) add a test. I.e. I believe your CL changed behavior here because STLDeleteElements is actually ClearButRememberToDeleteElementsToo. That this bug wasn't noticed by the tests suggest we need better test coverage.
The CQ bit was checked by avi@chromium.org to run a CQ dry run
https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... File net/spdy/spdy_read_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:53: void SpdyReadQueue::Clear() { On 2016/10/03 17:20:29, davidben wrote: > On 2016/10/03 16:58:47, Avi wrote: > > On 2016/10/03 16:16:31, davidben wrote: > > > queue_.clear(); > > > > > > +bnc: It's distressing that no test noticed this. Would you mind adding one? > > > > Is the request to add the clear call for me, or bnc@? I would prefer if this > > remained a refactoring-only CL. > > It's a request to add the clear() call plus a request for bnc@ to (in a separate > CL) add a test. I.e. I believe your CL changed behavior here because > STLDeleteElements is actually ClearButRememberToDeleteElementsToo. That this bug > wasn't noticed by the tests suggest we need better test coverage. Re adding clear(), d'oh! Yeah, that's my fault.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM with nit. Also fine with respect to internal code. https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... File net/spdy/spdy_read_queue.cc (right): https://codereview.chromium.org/2386883002/diff/40001/net/spdy/spdy_read_queu... net/spdy/spdy_read_queue.cc:53: void SpdyReadQueue::Clear() { On 2016/10/03 16:16:31, davidben wrote: > queue_.clear(); > > +bnc: It's distressing that no test noticed this. Would you mind adding one? Good idea. I will add a test after this CL lands. https://codereview.chromium.org/2386883002/diff/80001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.cc (right): https://codereview.chromium.org/2386883002/diff/80001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:106: auto& queue = queue_[priority]; I think it would be better not to use |auto| here: the type name is not particularily long or complicated, and type cannot be easily deduced from context, only if one consults the header file. https://codereview.chromium.org/2386883002/diff/80001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:128: auto& queue = queue_[i]; Same here.
The CQ bit was checked by avi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bnc@chromium.org Link to the patchset: https://codereview.chromium.org/2386883002/#ps100001 (title: "nits")
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.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Remove stl_util's deletion functions from net/spdy/. BUG=555865 ========== to ========== Remove stl_util's deletion functions from net/spdy/. BUG=555865 Committed: https://crrev.com/ae73dbcef87d02504f6b3092a30bc6488ffd155e Cr-Commit-Position: refs/heads/master@{#422495} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ae73dbcef87d02504f6b3092a30bc6488ffd155e Cr-Commit-Position: refs/heads/master@{#422495}
Message was sent while issue was closed.
https://codereview.chromium.org/2386883002/diff/80001/net/spdy/spdy_write_que... File net/spdy/spdy_write_queue.cc (right): https://codereview.chromium.org/2386883002/diff/80001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:106: auto& queue = queue_[priority]; On 2016/10/03 18:06:45, Bence wrote: > I think it would be better not to use |auto| here: the type name is not > particularily long or complicated, and type cannot be easily deduced from > context, only if one consults the header file. Done. https://codereview.chromium.org/2386883002/diff/80001/net/spdy/spdy_write_que... net/spdy/spdy_write_queue.cc:128: auto& queue = queue_[i]; On 2016/10/03 18:06:45, Bence wrote: > Same here. Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
