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

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons marked an inline comment as done.
malcolm.parsons added inline comments.


================
Comment at: include/clang/Sema/ScopeInfo.h:457
+    /// lambda.
+    bool Used = false;
+
----------------
aaron.ballman wrote:
> arphaman wrote:
> > malcolm.parsons wrote:
> > > Should this be moved into one of the `PointerIntPair`s?
> > I'm not sure.. If we needed other capturing information in the future I would say that this should be its own field, but I'm not aware of anything else that's needed for this class. So I guess it would be better to pack it into `VarAndNestedAndThis`, but I wouldn't say it's a deal breaker.
> I'm not keen on inconsistently initializating data members; can you perform this initialization in the constructor instead?
I'm not keen on repeating the initialization in every constructor.


================
Comment at: lib/Sema/SemaLambda.cpp:1479
     for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
-      LambdaScopeInfo::Capture From = LSI->Captures[I];
+      LambdaScopeInfo::Capture &From = LSI->Captures[I];
       assert(!From.isBlockCapture() && "Cannot capture __block variables");
----------------
aaron.ballman wrote:
> Why does `From` need to be a non-const reference?
It's not related to the warning; it looks like an unnecessary copy.


================
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}}
+
----------------
Quuxplusone wrote:
> 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)`.
Does "lambda capture 'i' is not odr-used" make more sense?


================
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}}
+
----------------
Quuxplusone wrote:
> 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".
I was planning to check for that; thanks for the reminder.


https://reviews.llvm.org/D28467