Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646873002:
Fix unused lambda captures in //net. (Closed)
DescriptionFix unused lambda captures in //net.
Clang just got a new warning about unused lambda captures,
and that requires us to clean all places with this issue
across all the Chromium code base. This CL fixes all such
cases in //net.
An example of a code that is no longer considered normal is
auto g = [i, j]() { printf("%d\n", i); };
in which j is never used in the lambda and therefore its capture is unnecessary.
BUG=681136
Review-Url: https://codereview.chromium.org/2646873002
Cr-Commit-Position: refs/heads/master@{#445087}
Committed: https://chromium.googlesource.com/chromium/src/+/17305ea8ba2799834b6eda071348c7724e2c68be
Patch Set 1 #Patch Set 2 : constexpr to satisfy MSVC++ #Patch Set 3 : second attempt to satisfy MSVC++. #
Total comments: 2
Messages
Total messages: 24 (12 generated)
The CQ bit was checked by krasin@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 checked by krasin@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...
krasin@chromium.org changed reviewers: + mmenke@chromium.org
mmenke@chromium.org changed reviewers: + rch@chromium.org
[+rch]: Mind taking this one?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
krasin@chromium.org changed reviewers: + eroman@chromium.org
Hi Roman, can you please take a look at this tiny cleanup CL? It currently blocks updating Clang to newer version.
lgtm https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... File net/quic/test_tools/simulator/simulator_test.cc (right): https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... net/quic/test_tools/simulator/simulator_test.cc:312: static const QuicPacketCount packets_received = 1000; [optional] These should probably be renamed to kPacketsReceived to match the constants style. On the other hand given the other use of "const" in this file that would be inconsistent, so maybe best to leave current names.
On 2017/01/20 17:49:26, eroman (slow) wrote: > lgtm Thank you for the fast review! > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > File net/quic/test_tools/simulator/simulator_test.cc (right): > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > net/quic/test_tools/simulator/simulator_test.cc:312: static const > QuicPacketCount packets_received = 1000; > [optional] These should probably be renamed to kPacketsReceived to match the > constants style. On the other hand given the other use of "const" in this file > that would be inconsistent, so maybe best to leave current names.
https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... File net/quic/test_tools/simulator/simulator_test.cc (right): https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... net/quic/test_tools/simulator/simulator_test.cc:312: static const QuicPacketCount packets_received = 1000; On 2017/01/20 17:49:25, eroman (slow) wrote: > [optional] These should probably be renamed to kPacketsReceived to match the > constants style. On the other hand given the other use of "const" in this file > that would be inconsistent, so maybe best to leave current names. Let's leave it as is. Yesterday, in a similar case I was asked to renamed a var from kSomethingSomething to something_something. Which means there is no agreement between the owners, and doing fewer back-n-forth changes is probably wise.
The CQ bit was checked by krasin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484935346670980,
"parent_rev": "1de5a2d845e43bed00617402858e16ed88d48770", "commit_rev":
"17305ea8ba2799834b6eda071348c7724e2c68be"}
Message was sent while issue was closed.
Description was changed from
==========
Fix unused lambda captures in //net.
Clang just got a new warning about unused lambda captures,
and that requires us to clean all places with this issue
across all the Chromium code base. This CL fixes all such
cases in //net.
An example of a code that is no longer considered normal is
auto g = [i, j]() { printf("%d\n", i); };
in which j is never used in the lambda and therefore its capture is unnecessary.
BUG=681136
==========
to
==========
Fix unused lambda captures in //net.
Clang just got a new warning about unused lambda captures,
and that requires us to clean all places with this issue
across all the Chromium code base. This CL fixes all such
cases in //net.
An example of a code that is no longer considered normal is
auto g = [i, j]() { printf("%d\n", i); };
in which j is never used in the lambda and therefore its capture is unnecessary.
BUG=681136
Review-Url: https://codereview.chromium.org/2646873002
Cr-Commit-Position: refs/heads/master@{#445087}
Committed:
https://chromium.googlesource.com/chromium/src/+/17305ea8ba2799834b6eda071348...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/17305ea8ba2799834b6eda071348...
Message was sent while issue was closed.
On 2017/01/20 18:02:20, krasin1 wrote: > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > File net/quic/test_tools/simulator/simulator_test.cc (right): > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > net/quic/test_tools/simulator/simulator_test.cc:312: static const > QuicPacketCount packets_received = 1000; > On 2017/01/20 17:49:25, eroman (slow) wrote: > > [optional] These should probably be renamed to kPacketsReceived to match the > > constants style. On the other hand given the other use of "const" in this file > > that would be inconsistent, so maybe best to leave current names. > > Let's leave it as is. Yesterday, in a similar case I was asked to renamed a var > from kSomethingSomething to something_something. Which means there is no > agreement between the owners, and doing fewer back-n-forth changes is probably > wise. This was in a net/ file?
Message was sent while issue was closed.
On 2017/01/20 18:09:31, mmenke wrote: > On 2017/01/20 18:02:20, krasin1 wrote: > > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > > File net/quic/test_tools/simulator/simulator_test.cc (right): > > > > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > > net/quic/test_tools/simulator/simulator_test.cc:312: static const > > QuicPacketCount packets_received = 1000; > > On 2017/01/20 17:49:25, eroman (slow) wrote: > > > [optional] These should probably be renamed to kPacketsReceived to match the > > > constants style. On the other hand given the other use of "const" in this > file > > > that would be inconsistent, so maybe best to leave current names. > > > > Let's leave it as is. Yesterday, in a similar case I was asked to renamed a > var > > from kSomethingSomething to something_something. Which means there is no > > agreement between the owners, and doing fewer back-n-forth changes is probably > > wise. > > This was in a net/ file? It was in an ash/ file: https://codereview.chromium.org/2643853003/
Message was sent while issue was closed.
On 2017/01/20 18:15:51, krasin1 wrote: > On 2017/01/20 18:09:31, mmenke wrote: > > On 2017/01/20 18:02:20, krasin1 wrote: > > > > > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > > > File net/quic/test_tools/simulator/simulator_test.cc (right): > > > > > > > > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > > > net/quic/test_tools/simulator/simulator_test.cc:312: static const > > > QuicPacketCount packets_received = 1000; > > > On 2017/01/20 17:49:25, eroman (slow) wrote: > > > > [optional] These should probably be renamed to kPacketsReceived to match > the > > > > constants style. On the other hand given the other use of "const" in this > > file > > > > that would be inconsistent, so maybe best to leave current names. > > > > > > Let's leave it as is. Yesterday, in a similar case I was asked to renamed a > > var > > > from kSomethingSomething to something_something. Which means there is no > > > agreement between the owners, and doing fewer back-n-forth changes is > probably > > > wise. > > > > This was in a net/ file? > > It was in an ash/ file: https://codereview.chromium.org/2643853003/ Different directories tend to use different styles, and adhere to different parts of the official C++ to different degrees. Generally best to match local style (Not suggesting you submit another separate CL for that or anything, just a general comment on varying styles)
Message was sent while issue was closed.
On 2017/01/20 18:24:05, mmenke wrote: > On 2017/01/20 18:15:51, krasin1 wrote: > > On 2017/01/20 18:09:31, mmenke wrote: > > > On 2017/01/20 18:02:20, krasin1 wrote: > > > > > > > > > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > > > > File net/quic/test_tools/simulator/simulator_test.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2646873002/diff/40001/net/quic/test_tools/sim... > > > > net/quic/test_tools/simulator/simulator_test.cc:312: static const > > > > QuicPacketCount packets_received = 1000; > > > > On 2017/01/20 17:49:25, eroman (slow) wrote: > > > > > [optional] These should probably be renamed to kPacketsReceived to match > > the > > > > > constants style. On the other hand given the other use of "const" in > this > > > file > > > > > that would be inconsistent, so maybe best to leave current names. > > > > > > > > Let's leave it as is. Yesterday, in a similar case I was asked to renamed > a > > > var > > > > from kSomethingSomething to something_something. Which means there is no > > > > agreement between the owners, and doing fewer back-n-forth changes is > > probably > > > > wise. > > > > > > This was in a net/ file? > > > > It was in an ash/ file: https://codereview.chromium.org/2643853003/ > > Different directories tend to use different styles, and adhere to different > parts of the official C++ to different degrees. Generally best to match local > style (Not suggesting you submit another separate CL for that or anything, just > a general comment on varying styles) Ack. |
