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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
