[PATCH] D28467: [Sema] Add warning for unused lambda captures

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:17
+  auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture 'i' is not used}}
+  auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; // expected-warning{{lambda capture 'i' is not used}}
+
----------------
aaron.ballman wrote:
> This does not match the behavior for other -Wunused flags. e.g.,
> ```
> void f() {
>   int i;
>   (void)sizeof(i);
> }
> ```
> I don't think diagnosing this test case is correct.
I see some value to maybe diagnosing *something* here. For example, `[] { return sizeof(i); }` would produce the same result as `[i] { return sizeof(i); }` but with one fewer capture, so removing the `i` might be seen as an improvement.

But I'm not sure how to convey this information to the user. You could say "variable `i` used in unevaluated context refers to the `i` in the outer scope, not the captured `i`"... except that I think that would be false. Given that you *have* captured an `i`, `sizeof(i)` definitely refers to the captured one AFAIK. The fact that the captured `i` shadows an `i` in the outer scope is irrelevant --- in fact the user is *expecting* to shadow the outer `i`.

Perhaps it would be appropriate to reword the diagnostic to "lambda captures variable `i` unnecessarily".  I would also lean toward splitting it into two diagnostics — one for "this capture is unnecessary" (as in this example) and one for "this capture doesn't even appear lexically in the body of the lambda". Not sure how other people would feel about that.

You should add some test cases with `decltype(i)` for the same reason as `sizeof(i)`.


================
Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:26
+  auto explicit_initialized_value_used = [j = 1] { return j + 1; };
+  auto explicit_initialized_value_unused = [j = 1] {}; // expected-warning{{lambda capture 'j' is not used}}
+
----------------
Would this still produce a diagnostic if `decltype(j)` were something complicated like `lock_guard` or `string`? Presumably it should do the same thing, more or less, as the other -Wunused diagnostics, which I think is "don't warn if the constructor/destructor might actually do important work".


https://reviews.llvm.org/D28467