rjmccall added inline comments.
================ Comment at: lib/Sema/SemaLambda.cpp:959 + ? diag::ext_equals_this_lambda_capture_cxx2a + : diag::warn_cxx1z_compat_equals_this_lambda_capture); ---------------- hamzasood wrote: > faisalv wrote: > > Shouldn't we try and hit the 'continue' (that u deleted) if warnings (and > > extension warnings) are turned into errors? > That's an interesting scenario which admittedly I hadn't considered. > > I based this implementation on the [[ > https://github.com/llvm-mirror/clang/blob/7602b13a8e8b5656afd6327d112b76b39f836e5b/lib/Sema/SemaLambda.cpp#L935 > | '*this' capture handling ]] from the same loop. When a '*this' capture is > seen pre-C++1z, an extension warning is emitted and then the capture is > processed as normal (i.e. without consideration of that warning potentially > becoming an error). > > I also looked at other places where extension warnings are emitted and I > couldn't find any special handling for warnings becoming errors. Yeah, I doubt there's a single place in the compiler where we stop processing code when warnings are turned into errors. ================ Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}} (void)[i, i]{ }; // expected-error{{'i' can appear only once in a capture list}} ---------------- rjmccall wrote: > hamzasood wrote: > > rjmccall wrote: > > > hamzasood wrote: > > > > rjmccall wrote: > > > > > hamzasood wrote: > > > > > > rjmccall wrote: > > > > > > > Shouldn't you only be accepting this in C++2a mode? > > > > > > I'm not sure what the system is with allowing future language > > > > > > features as extensions, but I noticed that [*this] capture is > > > > > > allowed as an extension pre-C++17 so I figured it would make sense > > > > > > for [=, this] to also be allowed as an extension (since the > > > > > > proposal mentions how it's meant to increase code clarify in the > > > > > > presence of [*this]). > > > > > Surely there should at least be an on-by-default extension warning? > > > > > The behavior we're using sounds a lot more like we're treating this > > > > > as a bug-fix in the standard than a new feature. Richard, can you > > > > > weigh in here? > > > > The extension warning for this (ext_equals_this_lambda_capture_cxx2a) > > > > is on by default. > > > Why did the diagnostic disappear from this file, then? > > That file is for FixIt hints, which I don't think make much sense for an > > extension warning (and I couldn't find any other extension warnings that > > offer FixIt hints) > Sure, it's reasonable for this specific test to not test the warning. > However, since I don't see anything in this test that actually turns off the > warning, and since you have not in fact added any tests that verify that the > warning is ever turned on, I suspect that it is actually not being emitted. I'm sorry, I see that you've added an explicit test for the warning, but I still don't understand why the warning is not emitted in this file. -verify normally verifies all diagnostics, and this file is tested with -std=c++11. Is there some special behavior of -fixit that disables warnings, or ignores them in -verify mode? https://reviews.llvm.org/D36572 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
