[cfe-commits] r138821 - in /cfe/trunk: include/clang/AST/ include/clang/Basic/ include/clang/Sema/ lib/AST/ lib/CodeGen/ lib/Sema/ test/CXX/dcl.decl/dcl.init/ test/CXX/special/class.copy/ test/CXX/special/class.inhctor/ test/CXX/stmt.stmt/stmt.iter/stmt.ranged/ test/CXX/temp/temp.decls/temp.variadic/ test/SemaCXX/

[Sebastian Redl]

On 30.08.2011, at 23:06, Douglas Gregor wrote:

> 
> On Aug 30, 2011, at 12:58 PM, Sebastian Redl wrote:
> 
>> Author: cornedbee
>> Date: Tue Aug 30 14:58:05 2011
>> New Revision: 138821
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=138821&view=rev
>> Log:
>> Declare and define implicit move constructor and assignment operator.
>> 
>> This makes the code duplication of implicit special member handling even worse,
>> but the cleanup will have to come later. For now, this works.
>> Follow-up with tests for explicit defaulting and enabling the __has_feature
>> flag to come.
> 
> Awesome!
> 
>> 
>> Modified: cfe/trunk/include/clang/AST/DeclCXX.h
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/DeclCXX.h?rev=138821&r1=138820&r2=138821&view=diff
>> ==============================================================================
>> --- cfe/trunk/include/clang/AST/DeclCXX.h (original)
>> +++ cfe/trunk/include/clang/AST/DeclCXX.h Tue Aug 30 14:58:05 2011
>> @@ -468,6 +468,14 @@
>> /// \brief Whether we have already declared a destructor within the class.
>> bool DeclaredDestructor : 1;
>> 
>> +    /// \brief Whether an implicit move constructor was attempted to be declared
>> +    /// but would have been deleted.
>> +    bool FailedImplicitMoveConstructor : 1;
>> +
>> +    /// \brief Whether an implicit move assignment operator was attempted to be
>> +    /// declared but would have been deleted.
>> +    bool FailedImplicitMoveAssignment : 1;
> 
> These bits aren't serialized to the AST file?
> 

Ah, not yet. Good catch. I'm rusty ...

>> Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=138821&r1=138820&r2=138821&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Tue Aug 30 14:58:05 2011
>> @@ -1860,6 +1860,19 @@
>>                                               EllipsisLoc);
>> }
>> 
>> +// Create a static_cast\<T&&>(expr).
>> +static Expr *CastForMoving(Sema &SemaRef, Expr *E) {
>> +  QualType ExprType = E->getType();
>> +  QualType TargetType = SemaRef.Context.getRValueReferenceType(ExprType);
>> +  SourceLocation ExprLoc = E->getLocStart();
>> +  TypeSourceInfo *TargetLoc = SemaRef.Context.getTrivialTypeSourceInfo(
>> +      TargetType, ExprLoc);
>> +
>> +  return SemaRef.BuildCXXNamedCast(ExprLoc, tok::kw_static_cast, TargetLoc, E,
>> +                                   SourceRange(ExprLoc, ExprLoc),
>> +                                   E->getSourceRange()).take();
>> +}
>> +
>> /// ImplicitInitializerKind - How an implicit base or member initializer should
>> /// initialize its base or member.
>> enum ImplicitInitializerKind {
>> @@ -1890,7 +1903,9 @@
>> break;
>> }
>> 
>> +  case IIK_Move:
>> case IIK_Copy: {
>> +    bool Moving = ImplicitInitKind == IIK_Move;
>> ParmVarDecl *Param = Constructor->getParamDecl(0);
>> QualType ParamType = Param->getType().getNonReferenceType();
>> 
>> @@ -1898,17 +1913,22 @@
>>   DeclRefExpr::Create(SemaRef.Context, NestedNameSpecifierLoc(), Param, 
>>                       Constructor->getLocation(), ParamType,
>>                       VK_LValue, 0);
>> -    
>> +
>> // Cast to the base class to avoid ambiguities.
>> QualType ArgTy = 
>>   SemaRef.Context.getQualifiedType(BaseSpec->getType().getUnqualifiedType(), 
>>                                    ParamType.getQualifiers());
>> 
>> +    if (Moving) {
>> +      CopyCtorArg = CastForMoving(SemaRef, CopyCtorArg);
>> +    }
>> +
>> CXXCastPath BasePath;
>> BasePath.push_back(BaseSpec);
>> CopyCtorArg = SemaRef.ImpCastExprToType(CopyCtorArg, ArgTy,
>>                                         CK_UncheckedDerivedToBase,
>> -                                            VK_LValue, &BasePath).take();
>> +                                            Moving ? VK_RValue : VK_LValue,
>> +                                            &BasePath).take();
> 
> Shouldn't this be VK_XValue rather than VK_RValue?
> 

Yes.

>> 
>> @@ -1964,11 +1987,16 @@
>>   DeclRefExpr::Create(SemaRef.Context, NestedNameSpecifierLoc(), Param, 
>>                       Loc, ParamType, VK_LValue, 0);
>> 
>> +    if (Moving) {
>> +      MemberExprBase = CastForMoving(SemaRef, MemberExprBase);
>> +    }
>> +
>> // Build a reference to this field within the parameter.
>> CXXScopeSpec SS;
>> LookupResult MemberLookup(SemaRef, Field->getDeclName(), Loc,
>>                           Sema::LookupMemberName);
>> -    MemberLookup.addDecl(Indirect? cast<ValueDecl>(Indirect) : cast<ValueDecl>(Field), AS_public);
>> +    MemberLookup.addDecl(Indirect ? cast<ValueDecl>(Indirect)
>> +                                  : cast<ValueDecl>(Field), AS_public);
>> MemberLookup.resolveKind();
>> ExprResult CopyCtorArg 
>>   = SemaRef.BuildMemberReferenceExpr(MemberExprBase,
>> @@ -1980,7 +2008,14 @@
>>                                      /*TemplateArgs=*/0);    
>> if (CopyCtorArg.isInvalid())
>>   return true;
>> -    
>> +
>> +    // C++11 [class.copy]p15:
>> +    //   - if a member m has rvalue reference type T&&, it is direct-initialized
>> +    //     with static_cast<T&&>(x.m);
>> +    if (RefersToRValueRef(CopyCtorArg.get())) {
>> +      CopyCtorArg = CastForMoving(SemaRef, CopyCtorArg.take());
>> +    }
> 
> The name "CopyCtorArg" starts to lose some meaning here…
> 

True, I just didn't want to clutter the patch with that change. Cleanup will come.

>> @@ -3332,7 +3376,7 @@
>>                      Context.VoidTy, 0, 0, EPI)->getAs<FunctionProtoType>();
>> 
>> QualType ArgType = OperType->getArgType(0);
>> -  if (!ArgType->isReferenceType()) {
>> +  if (!ArgType->isLValueReferenceType()) {
>> Diag(MD->getLocation(), diag::err_defaulted_copy_assign_not_ref);
>> HadError = true;
>> } else {
>> @@ -3384,6 +3428,155 @@
>> }
>> }
>> 
>> +void Sema::CheckExplicitlyDefaultedMoveConstructor(CXXConstructorDecl *CD) {
>> +  assert(CD->isExplicitlyDefaulted() && CD->isMoveConstructor());
>> +
>> +  // Whether this was the first-declared instance of the constructor.
>> +  bool First = CD == CD->getCanonicalDecl();
>> +
>> +  bool HadError = false;
>> +  if (CD->getNumParams() != 1) {
>> +    Diag(CD->getLocation(), diag::err_defaulted_move_ctor_params)
>> +      << CD->getSourceRange();
>> +    HadError = true;
>> +  }
>> +
>> +  ImplicitExceptionSpecification Spec(
>> +      ComputeDefaultedMoveCtorExceptionSpec(CD->getParent()));
>> +
>> +  FunctionProtoType::ExtProtoInfo EPI = Spec.getEPI();
>> +  const FunctionProtoType *CtorType = CD->getType()->getAs<FunctionProtoType>(),
>> +                          *ExceptionType = Context.getFunctionType(
>> +                         Context.VoidTy, 0, 0, EPI)->getAs<FunctionProtoType>();
>> +
>> +  // Check for parameter type matching.
>> +  // This is a move ctor so we know it's a cv-qualified rvalue reference to T.
>> +  QualType ArgType = CtorType->getArgType(0);
>> +  if (ArgType->getPointeeType().isVolatileQualified()) {
>> +    Diag(CD->getLocation(), diag::err_defaulted_move_ctor_volatile_param);
>> +    HadError = true;
>> +  }
>> +  if (ArgType->getPointeeType().isConstQualified()) {
>> +    Diag(CD->getLocation(), diag::err_defaulted_move_ctor_const_param);
>> +    HadError = true;
>> +  }
> 
> Shouldn't we just check for any qualifiers here? We wouldn't want it to be restrict-qualifier, or address-space-256-qualified, or any other ridiculous non-C++ qualifier.

Yes. And I also should fold the various diagnostics into one.

Thanks for the review.

Sebastian