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