Fix places that trigger no-unused-lambda-capture

Fix places that trigger no-unused-lambda-capture

no-unused-lambda-capture was suppressed, but it's been decided as desireable to stop suppressing it. This CL fixes places in the code that trigger it.

1. Some unnecessary captures removed.
2. s/constexpr/const when capturing a float by value - this is good enough to stop the error.
3. Complete removal of the constexpr/const-modifier for int-types as a workaround.

BUG=webrtc:7133
Review-Url: https://codereview.webrtc.org/3005433002
Cr-Commit-Position: refs/heads/master@{#19462}
Committed: https://chromium.googlesource.com/external/webrtc/+/1cc5fc3ebfacefe8326abb6c3367be6ddf524957

[eladalon, 2017-08-23 09:33:00]

Description was changed from

==========
Fix places that trigger no-unused-lambda-capture

no-unused-lambda-capture was suppressed, but it's been decided as desireable to
stop suppressing it. This CL fixes places in the code that trigger it.

1. Some unnecessary captures removed.
2. s/constexpr/const when capturing a float by value - this is good enough to
stop the error.
3. Complete removal of the constexpr/const-modifier for int-types as a
workaround.

BUG=webrtc:7133
==========

to

==========
Fix places that trigger no-unused-lambda-capture

no-unused-lambda-capture was suppressed, but it's been decided as desireable to
stop suppressing it. This CL fixes places in the code that trigger it.

1. Some unnecessary captures removed.
2. s/constexpr/const when capturing a float by value - this is good enough to
stop the error.
3. Complete removal of the constexpr/const-modifier for int-types as a
workaround.

BUG=webrtc:7133
==========

[eladalon, 2017-08-23 09:33:01]

eladalon@webrtc.org changed reviewers:
+ charujain@webrtc.org, nisse@webrtc.org, stefan@webrtc.org

[eladalon, 2017-08-23 09:33:38]

PTAL

[stefan-webrtc, 2017-08-23 09:37:25]

https://codereview.webrtc.org/3005433002/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/3005433002/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1214: int kLastPacketId = 100;
Can't we at least make them const?

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests.cc
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:4276: int kFrameMaxWidth = 320;
const?

[nisse-webrtc, 2017-08-23 09:42:03]

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests.cc
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:4273: // require them to be captured in the
lambda, and other compilers complain
If I understood you correctly, it's some windows compiler which is the exception
(and probably buggy), requiring the constants to be included in the capture
list. So if it's a workaround for a compiler bug, be a bit more explicit about
which compiler is broken.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:510: const float
kScale = 1.0f / kFftLengthBy2;
Could add a comment here too, on why constexpr doesn't work.

[charujain1, 2017-08-23 09:57:06]

On 2017/08/23 09:42:03, nisse-webrtc wrote:
>
https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests.cc
> File webrtc/video/end_to_end_tests.cc (right):
> 
>
https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests...
> webrtc/video/end_to_end_tests.cc:4273: // require them to be captured in the
> lambda, and other compilers complain
> If I understood you correctly, it's some windows compiler which is the
exception
> (and probably buggy), requiring the constants to be included in the capture
> list. So if it's a workaround for a compiler bug, be a bit more explicit about
> which compiler is broken.
> 
>
https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):
> 
>
https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:510: const float
> kScale = 1.0f / kFftLengthBy2;
> Could add a comment here too, on why constexpr doesn't work.

Could you please integrate this change
(https://codereview.webrtc.org/2999403002/) in this CL and run CQ again to be
sure.

[eladalon, 2017-08-23 10:26:26]

Charu, I suggest I check in combination with Henrik's CL locally, but land my CL
with only my changes, to keep history cleaner, and things easier to revert.

https://codereview.webrtc.org/3005433002/diff/1/webrtc/pc/channel_unittest.cc
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/3005433002/diff/1/webrtc/pc/channel_unittest.cc...
webrtc/pc/channel_unittest.cc:1214: int kLastPacketId = 100;
On 2017/08/23 09:37:24, stefan-webrtc wrote:
> Can't we at least make them const?

No. The Mac compiler complains.

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests.cc
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:4273: // require them to be captured in the
lambda, and other compilers complain
On 2017/08/23 09:42:02, nisse-webrtc wrote:
> If I understood you correctly, it's some windows compiler which is the
exception
> (and probably buggy), requiring the constants to be included in the capture
> list. So if it's a workaround for a compiler bug, be a bit more explicit about
> which compiler is broken.

Might take some time. I suggest lgtm-izing this CL, unblocking some internal
projects, and landing a fix of the comment afterwards. (I'll get to work finding
out which compiler it is right away.)

https://codereview.webrtc.org/3005433002/diff/1/webrtc/video/end_to_end_tests...
webrtc/video/end_to_end_tests.cc:4276: int kFrameMaxWidth = 320;
On 2017/08/23 09:37:25, stefan-webrtc wrote:
> const?

Same answer.

[nisse-webrtc, 2017-08-23 10:28:39]

lgtm

[kjellander_webrtc, 2017-08-23 10:32:08]

kjellander@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[kjellander_webrtc, 2017-08-23 10:32:08]

lgtm I can do a try with my https://codereview.webrtc.org/2999403002/ after this
lands.

I think you need more owners approvals though. Ping kwiberg?

[stefan-webrtc, 2017-08-23 10:42:14]

lgtm

[eladalon, 2017-08-23 10:45:39]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-08-23 10:45:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-08-23 11:15:19]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1503485139119310,
"parent_rev": "3df87523aa3e3b6f9c751d99280ba865f22ccdc2", "commit_rev":
"1cc5fc3ebfacefe8326abb6c3367be6ddf524957"}

[commit-bot: I haz the power, 2017-08-23 11:15:31]

Description was changed from

==========
Fix places that trigger no-unused-lambda-capture

no-unused-lambda-capture was suppressed, but it's been decided as desireable to
stop suppressing it. This CL fixes places in the code that trigger it.

1. Some unnecessary captures removed.
2. s/constexpr/const when capturing a float by value - this is good enough to
stop the error.
3. Complete removal of the constexpr/const-modifier for int-types as a
workaround.

BUG=webrtc:7133
==========

to

==========
Fix places that trigger no-unused-lambda-capture

no-unused-lambda-capture was suppressed, but it's been decided as desireable to
stop suppressing it. This CL fixes places in the code that trigger it.

1. Some unnecessary captures removed.
2. s/constexpr/const when capturing a float by value - this is good enough to
stop the error.
3. Complete removal of the constexpr/const-modifier for int-types as a
workaround.

BUG=webrtc:7133

Review-Url: https://codereview.webrtc.org/3005433002
Cr-Commit-Position: refs/heads/master@{#19462}
Committed:
https://chromium.googlesource.com/external/webrtc/+/1cc5fc3ebfacefe8326abb6c3...
==========

[commit-bot: I haz the power, 2017-08-23 11:15:32]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/1cc5fc3ebfacefe8326abb6c3...

[kwiberg-webrtc, 2017-08-23 11:35:59]

kwiberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[kwiberg-webrtc, 2017-08-23 11:36:00]

Sorry, it seems I was too slow (had to pause reviewing to have a meeting). I
have some comments that might be useful, though.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:512: [kScale](float&
a) { a *= kScale; });
Does it work if you change the capture list to [&] instead? That's allowed since
the lambda doesn't outlive the scope in which it is created.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1214: int kLastPacketId = 100;
Shouldn't they at least be const?

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4287: int kFrameRate = 15;
const?

[eladalon, 2017-08-23 11:42:53]

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:510: const float
kScale = 1.0f / kFftLengthBy2;
On 2017/08/23 09:42:02, nisse-webrtc wrote:
> Could add a comment here too, on why constexpr doesn't work.

IMHO, it would be overkill to fill the code with too many comments about this.
If someone ever tries to "fix" this, the trybots would prevent that. Or, if it
ever starts passing, and someone fixes it then - great.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:512: [kScale](float&
a) { a *= kScale; });
On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> Does it work if you change the capture list to [&] instead? That's allowed
since
> the lambda doesn't outlive the scope in which it is created.

You can't capture a constexpr by reference, can you?

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1214: int kLastPacketId = 100;
On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> Shouldn't they at least be const?

No, it doesn't work if they are. Please see my answer to Stefan about this.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/video/end_to_end_t...
File webrtc/video/end_to_end_tests.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/video/end_to_end_t...
webrtc/video/end_to_end_tests.cc:4287: int kFrameRate = 15;
On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> const?

Similarly.

[kwiberg-webrtc, 2017-08-23 11:56:40]

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:512: [kScale](float&
a) { a *= kScale; });
On 2017/08/23 11:42:53, eladalon wrote:
> On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> > Does it work if you change the capture list to [&] instead? That's allowed
> since
> > the lambda doesn't outlive the scope in which it is created.
> 
> You can't capture a constexpr by reference, can you?

Yes, you can. Unlike preprocessor constants and enum values, they are lvalues,
so you can take their address etc.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1214: int kLastPacketId = 100;
On 2017/08/23 11:42:53, eladalon wrote:
> On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> > Shouldn't they at least be const?
> 
> No, it doesn't work if they are. Please see my answer to Stefan about this.

Ugh. This is really bad.

What if we make them enums, and don't mention them in the capture list:

  enum : uint16_t { kLocalNetId = 1 };
  enum : uint16_t { kRemoteNetId = 2 };
  enum : int { kLastPacketId = 100 };
  ...
  network_thread_->Invoke<void>(RTC_FROM_HERE, [this] { ... });

[eladalon, 2017-08-23 12:17:30]

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:512: [kScale](float&
a) { a *= kScale; });
On 2017/08/23 11:56:40, kwiberg-webrtc wrote:
> On 2017/08/23 11:42:53, eladalon wrote:
> > On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> > > Does it work if you change the capture list to [&] instead? That's allowed
> > since
> > > the lambda doesn't outlive the scope in which it is created.
> > 
> > You can't capture a constexpr by reference, can you?
> 
> Yes, you can. Unlike preprocessor constants and enum values, they are lvalues,
> so you can take their address etc.

Surprising, but OK. Nevertheless, I am not sure that would be an improvement of
the code. I think it would be better with const than with an unlimited capture.
Unless you feel strongly about this, I'd rather not make that change.

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
File webrtc/pc/channel_unittest.cc (right):

https://codereview.webrtc.org/3005433002/diff/20001/webrtc/pc/channel_unittes...
webrtc/pc/channel_unittest.cc:1214: int kLastPacketId = 100;
On 2017/08/23 11:56:40, kwiberg-webrtc wrote:
> On 2017/08/23 11:42:53, eladalon wrote:
> > On 2017/08/23 11:35:59, kwiberg-webrtc wrote:
> > > Shouldn't they at least be const?
> > 
> > No, it doesn't work if they are. Please see my answer to Stefan about this.
> 
> Ugh. This is really bad.
> 
> What if we make them enums, and don't mention them in the capture list:
> 
>   enum : uint16_t { kLocalNetId = 1 };
>   enum : uint16_t { kRemoteNetId = 2 };
>   enum : int { kLastPacketId = 100 };
>   ...
>   network_thread_->Invoke<void>(RTC_FROM_HERE, [this] { ... });

Interesting suggestion. I'm trying out another suggestion from Tommi. If that
one doesn't work, I'll explore this instead.