[PATCH] D19783: Fix cv-qualification of '*this' captures (and nasty bug PR27507 introduced by commit 263921 "Implement Lambda Capture of *this by Value as [=, *this]")

Faisal Vali via cfe-commits cfe-commits at lists.llvm.org
Wed May 18 21:49:49 PDT 2016


There are tests from the test file in my patch that don't pass(*) if
you just apply Oh's fix .  That's not surprising since Oh's patch only
meant to fix the crash, but not the 'cv' qualification issue of
'*this' that we didn't have to deal with prior to by-value captures of
'*this' and my initial implementation of feature patch
incorrectly/incompletely handled.  Also my patch attempts to simplify
the code by removing the storage of 'ThisType' in the Capture, since
it never gets queried, so no reason to store it (which was also what
was triggering the assertion initially).


(*) error: 'error' diagnostics seen but not expected:
  File F:\clang-trunk\llvm\tools\clang\test\semacxx\cxx1z-lambda-star-this.cpp
Line 88: static_assert failed
  File F:\clang-trunk\llvm\tools\clang\test\semacxx\cxx1z-lambda-star-this.cpp
Line 89: cannot assign to non-static data
 member within const member function 'foo'
  File F:\clang-trunk\llvm\tools\clang\test\semacxx\cxx1z-lambda-star-this.cpp
Line 91: static_assert failed
  File F:\clang-trunk\llvm\tools\clang\test\semacxx\cxx1z-lambda-star-this.cpp
Line 92: cannot assign to non-static data
 member within const member function 'foo'

Faisal Vali



On Wed, May 18, 2016 at 12:34 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> I'd also like to know whether there are cases that this patch addresses but
> Taewook Oh's patch does not, as the other patch involves a lot less
> complexity.
>
> On 18 May 2016 10:15 a.m., "Richard Smith via cfe-commits"
> <cfe-commits at lists.llvm.org> wrote:
>>
>> rsmith added inline comments.
>>
>> ================
>> Comment at: lib/Sema/SemaExprCXX.cpp:898
>> @@ +897,3 @@
>> +  // end of the TU) we need to be able to examine its enclosing lambdas
>> and so
>> +  // we use the DeclContext to get a hold of the ClosureClass and query
>> it for
>> +  // capture information.  The reason we don't just resort to always
>> using the
>> ----------------
>> No camel case for ClosureClass.
>>
>> ================
>> Comment at: lib/Sema/SemaExprCXX.cpp:910
>> @@ +909,3 @@
>> +      assert(IsFirstIteration);
>> +      assert(CurLSI->CallOperator->getParent()->getParent() == CurDC);
>> +      CurDC = CurLSI->CallOperator;
>> ----------------
>> Please add a comment explaining this, I have no idea what special case
>> you're checking for here.
>>
>> ================
>> Comment at: lib/Sema/SemaExprCXX.cpp:952
>> @@ +951,3 @@
>> +    while (Closure &&
>> +           IsThisCaptured(Closure, IsByCopyCapture, IsConstCapture)) {
>> +      if (IsByCopyCapture) {
>> ----------------
>> Should you really stop here if this is not captured? There could still be
>> a surrounding lambda with a mutable *this capture.
>>
>> ================
>> Comment at: lib/Sema/SemaExprCXX.cpp:1062
>> @@ -964,1 +1061,3 @@
>> +  Expr *This =
>> +      new (Context) CXXThisExpr(Loc, AdjustedThisTy, /*isImplicit*/
>> true);
>>    if (ByCopy) {
>> ----------------
>> faisalv wrote:
>> > Hmm - I wonder if instead of using AdjustedThisTy, I should always use
>> > 'ThisTy' since it is being used in the initializer expression, and should
>> > probably inherit its cv qualifiers from an enclosing lambda? correct?
>> Yes, don't use AdjustedThisTy here. You can test for this bug by giving
>> the class both a (const T&) and a (T&)=delete copy ctor.
>>
>>
>> http://reviews.llvm.org/D19783
>>
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


More information about the cfe-commits mailing list