Reenable -Wunused-lambda-capture warning.

Reenable -Wunused-lambda-capture warning.

This warning has been recently introduced into Clang,
and a lot of Clang ToT bots went red because Chromium
had a number of cases where lambdas had unused lambdas.

Note: there is an annoying bug in MSVC++ regarding to
capturing const values. Consider the code like:

const int N = 3;
auto g = [N](){ printf("%d\n", N); };
g();

Clang will issue a warning that N capture is unused. Okay, we remove it:

const int N = 3;
auto g = [](){ printf("%d\n", N); };
g();

but then MSVC++ is unhappy. And that's a bug in MSVC++, see
https://social.msdn.microsoft.com/Forums/SqlServer/4abf18bd-4ae4-4c72-ba3e-3b...
http://connect.microsoft.com/VisualStudio/feedback/details/725780/lambda-expr...
 (will require to sign in, and might not show its contents anyway)
http://wap.cpp.forum24.ru/?1-3-0-00000034-000-0-0 (in Russian; by the author of
the original bug report to Microsoft, Сыроежка)

The workaround is to have such constants static:

static const int N = 3;
auto g = [](){ printf("%d\n", N); };
g();

The cleanup is now complete, and it's time to reenable
the warning. I have tested it on Linux x86-64 by building
All targets. They succeeded.

Theoretically, some platform-specific code might still
have issues like that, but it should be trivial to fix
them, when they are detected by Clang ToT bots.

BUG=681136

[krasin1, 2017-01-24 21:35:36]

krasin@chromium.org changed reviewers:
+ dpranke@google.com

[krasin1, 2017-01-24 21:35:43]

The CQ bit was checked by krasin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-24 21:36:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nico, 2017-01-24 21:38:50]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-01-24 21:38:51]

How often did you run into the constexpr thing? If that's at all common, then
this warning will probably be somewhat annoying
(http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170123/183447.html)

[krasin1, 2017-01-24 21:50:13]

On 2017/01/24 21:38:51, Nico wrote:
> How often did you run into the constexpr thing? If that's at all common, then
> this warning will probably be somewhat annoying
> (http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170123/183447.html)

The issue was with const, not constexpr. In particular, consider the code like:

const int N = 3;
auto g = [N](){ printf("%d\n", N); };
g();

Clang will issue a warning that N capture is unused. Okay, we remove it:

const int N = 3;
auto g = [](){ printf("%d\n", N); };
g();

but then MSVC++ is unhappy. And that's a bug in MSVC++, see
https://social.msdn.microsoft.com/Forums/SqlServer/4abf18bd-4ae4-4c72-ba3e-3b...
http://connect.microsoft.com/VisualStudio/feedback/details/725780/lambda-expr...
 (will require to sign in, and might not show its contents anyway)
http://wap.cpp.forum24.ru/?1-3-0-00000034-000-0-0 (in Russian; by the author of
the original bug report to Microsoft, Сыроежка)

The workaround is to have such constants static (but not constexpr; I tried
constexpr and it failed; that's why you have memorized it)

It has happened 3 times during my cleanup. Not too often, but more than once. I
don't have a strong opinion on whether the warning is useful enough to be
enabled.

[Dirk Pranke, 2017-01-24 21:53:01]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2017-01-24 21:53:01]

I defer to Nico here.

[Nico, 2017-01-24 22:06:47]

Do you have a rough estimate how many non-const unused captures you removed? Is
it closer to 3 in 30 or 3 in 100 (or lower)?

(I'd change the CL description to mention this caveat and the recommended
workaround; that bit of information is probably more interesting to future
people than the history which is already in the bug.)

Rollout plan: Maybe it makes sense to wait for the next clang roll and then turn
it on after that? Else the next clang roll will update both the compiler and
(implicitly) turn on this warning, which increases roll risk. And once the
compiler is rev'd, this CL can be tested on the try bots. (I'm not sure this is
necessary, but it kind of feels like it might be a good idea.)

[krasin1, 2017-01-24 22:10:05]

Description was changed from

==========
Reenable -Wunused-lambda-capture warning.

This warning has been recently introduced into Clang,
and a lot of Clang ToT bots went red because Chromium
had a number of cases where lambdas had unused lambdas.

The cleanup is now complete, and it's time to reenable
the warning. I have tested it on Linux x86-64 by building
All targets. They succeeded.

Theoretically, some platform-specific code might still
have issues like that, but it should be trivial to fix
them, when they are detected by Clang ToT bots.

BUG=681136
==========

to

==========
Reenable -Wunused-lambda-capture warning.

This warning has been recently introduced into Clang,
and a lot of Clang ToT bots went red because Chromium
had a number of cases where lambdas had unused lambdas.

Note: there is an annoying bug in MSVC++ regarding to
capturing const values. Consider the code like:

const int N = 3;
auto g = [N](){ printf("%d\n", N); };
g();

Clang will issue a warning that N capture is unused. Okay, we remove it:

const int N = 3;
auto g = [](){ printf("%d\n", N); };
g();

but then MSVC++ is unhappy. And that's a bug in MSVC++, see
https://social.msdn.microsoft.com/Forums/SqlServer/4abf18bd-4ae4-4c72-ba3e-3b...
http://connect.microsoft.com/VisualStudio/feedback/details/725780/lambda-expr...
 (will require to sign in, and might not show its contents anyway)
http://wap.cpp.forum24.ru/?1-3-0-00000034-000-0-0 (in Russian; by the author of
the original bug report to Microsoft, Сыроежка)

The workaround is to have such constants static:

static const int N = 3;
auto g = [](){ printf("%d\n", N); };
g();

The cleanup is now complete, and it's time to reenable
the warning. I have tested it on Linux x86-64 by building
All targets. They succeeded.

Theoretically, some platform-specific code might still
have issues like that, but it should be trivial to fix
them, when they are detected by Clang ToT bots.

BUG=681136
==========

[krasin1, 2017-01-24 22:12:30]

On 2017/01/24 22:06:47, Nico wrote:
> Do you have a rough estimate how many non-const unused captures you removed?
Is
> it closer to 3 in 30 or 3 in 100 (or lower)?
It's about 3 in 30, yes.

> 
> (I'd change the CL description to mention this caveat and the recommended
> workaround; that bit of information is probably more interesting to future
> people than the history which is already in the bug.)
Done.

> 
> Rollout plan: Maybe it makes sense to wait for the next clang roll and then
turn
> it on after that? Else the next clang roll will update both the compiler and
> (implicitly) turn on this warning, which increases roll risk. And once the
> compiler is rev'd, this CL can be tested on the try bots. (I'm not sure this
is
> necessary, but it kind of feels like it might be a good idea.)

Sure, it makes sense. Let's not proceed with this CL at this time. Then, whoever
makes a roll, will disable the warning for all (not just ToT bots). And then
we'll enable it by running a proper CQ.

[commit-bot: I haz the power, 2017-01-24 23:40:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-24 23:40:36]

Dry run: This issue passed the CQ dry run.