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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:319
   InGroup<UnusedPrivateField>, DefaultIgnore;
+def warn_unused_lambda_capture: Warning<"lambda capture %0 is not odr-used">,
+  InGroup<UnusedLambdaCapture>, DefaultIgnore;
----------------
We do not use the term "odr-use" in any of our other frontend diagnostics; I don't think this is a term that users will actually understand. I think we should drop the term and just use "used".


================
Comment at: include/clang/Sema/ScopeInfo.h:457
+    /// lambda.
+    bool Used = false;
+
----------------
malcolm.parsons wrote:
> 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.
Consistency is the important bit (see the golden rule in http://llvm.org/docs/CodingStandards.html) -- if you want to move things to be in-class initializers, that would be a reasonable follow-up patch. For this patch, please put the initializer into the 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");
----------------
malcolm.parsons wrote:
> 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.
Thanks for adding the `const` qualifier.


================
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}}
+
----------------
malcolm.parsons wrote:
> Quuxplusone wrote:
> > malcolm.parsons wrote:
> > > 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?
> > > Does "lambda capture 'i' is not odr-used" make more sense?
> > 
> > That would completely satisfy *me*, for what that's worth. It admittedly doesn't match the other -Wunused diagnostics, but it is concise and correct — at least I assume it's correct. :)
> C++14 [expr.prim.lambda]p18:
> 
> > [ Note: An id-expression that is not an odr-use refers to the original entity, never to a member
> > of the closure type. Furthermore, such an id-expression does not cause the implicit capture of the entity.
> > — end note ]
Yes, that is correct; it's just incredibly confusing to most mortal programmers. Perhaps instead we can add extra information to the diagnostic telling the user that the variable need not be captured when it's in an unevaluated context (if possible). e.g.,

"lambda capture %0 %select{is not used|is not required to be captured for this use}1"

or something similar. My concern here is that coding standards tell users to explicitly capture variables that are referenced within the lambda, and the user will get a diagnostic that they assume is a false positive because most people don't know what an odr-use is. A quick search online found a handful of such coding standards, btw.


================
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}}
+
----------------
malcolm.parsons wrote:
> 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.
One more complex case:
```
#include <typeinfo>

struct B {
  virtual ~B() = default;
};

struct D : B {};

struct C {
  B& get_d() const;
  C get_c() const;
};

void f() {
  C c1, c2;
  [c1, c2] {
    (void)typeid(c1.get_d());
    (void)typeid(c2.get_c());
  }();
}
```
Hopefully this does not diagnose `c1` as being unused but still diagnoses `c2` as unused.


https://reviews.llvm.org/D28467