Fix/Improve SourceRange of explicitly defaulted members

[djasper]

Summary

When adding the implicit compound statement (required for Codegen?), the end location was previously overridden by the start location, probably based on the assumptions:

• The location of the implicit compound statement should be the member's location
• The compound statement if present is always the last element of a FunctionDecl

This patch changes the location of the compound statement to the member's end location, but I am open to other suggestions.

[rsmith]

lib/Sema/SemaDeclCXX.cpp

8885–8887

I think this is a reasonable approach. Do the other DefineImplicit* members also need this fix?

[djasper]

Fall back to getLocation() if getEndLoc() returns an invalid source location (this fixes a few tests for the default copy operators). Also fix a test that seems to have relied on the incorrectly calculated ranges (is the fix right?).

[rsmith]

When does the !isValid case happen? That sounds like it's probably a bug.

Anyway, LGTM.

[djasper]

It happens e.g. in test/SemaTemplate/instantiate-default-assignment-operator.cpp. Not sure whether it is a bug or not. I think if the entire member is implicit, getLocation() delivers a somewhat reasonable location whereas getLocEnd() (or EndRangeLoc) is never set.

[rsmith]

OK, I suppose that makes some sense (getSourceRange() would then be invalid for implicit things, but getLocation() would be a location to use for diagnostics).

lib/Sema/SemaDeclCXX.cpp

void Sema::DefineImplicitDefaultConstructor(SourceLocation CurrentLocation,
   if (SetCtorInitializers(Constructor, /*AnyErrors=*/false) ||
       Trap.hasErrorOccurred()) {
     Diag(CurrentLocation, diag::note_member_synthesized_at) 
       << CXXDefaultConstructor << Context.getTagDeclType(ClassDecl);
     Constructor->setInvalidDecl();
     return;
   }
 
-  SourceLocation Loc = Constructor->getLocation();
+  SourceLocation Loc = Constructor->getLocEnd().isValid()
+                           ? Constructor->getLocEnd()
+                           : Constructor->getLocation();
   Constructor->setBody(new (Context) CompoundStmt(Loc));
 
   Constructor->markUsed(Context);
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(Constructor);
   }
void Sema::DefineImplicitDestructor(SourceLocation CurrentLocation,
   if (CheckDestructor(Destructor) || Trap.hasErrorOccurred()) {
     Diag(CurrentLocation, diag::note_member_synthesized_at) 
       << CXXDestructor << Context.getTagDeclType(ClassDecl);
 
     Destructor->setInvalidDecl();
     return;
   }
 
-  SourceLocation Loc = Destructor->getLocation();
+  SourceLocation Loc = Destructor->getLocEnd().isValid()
+                           ? Destructor->getLocEnd()
+                           : Destructor->getLocation();
   Destructor->setBody(new (Context) CompoundStmt(Loc));
   Destructor->markUsed(Context);
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(Destructor);
   }
 }
void Sema::DefineImplicitCopyAssignment(SourceLocation CurrentLocation,
   QualType OtherRefType = Other->getType();
   if (const LValueReferenceType *OtherRef
                                 = OtherRefType->getAs<LValueReferenceType>()) {
     OtherRefType = OtherRef->getPointeeType();
     OtherQuals = OtherRefType.getQualifiers();
   }
   
   // Our location for everything implicitly-generated.
-  SourceLocation Loc = CopyAssignOperator->getLocation();
-  
+  SourceLocation Loc = CopyAssignOperator->getLocEnd().isValid()
+                           ? CopyAssignOperator->getLocEnd()
+                           : CopyAssignOperator->getLocation();
   // Builds a DeclRefExpr for the "other" object.
   RefBuilder OtherRef(Other, OtherRefType);
 
   // Builds the "this" pointer.
   ThisBuilder This;
   
   // Assign base classes.
   bool Invalid = false;
void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation,
   // The parameter for the "other" object, which we are move from.
   ParmVarDecl *Other = MoveAssignOperator->getParamDecl(0);
   QualType OtherRefType = Other->getType()->
       getAs<RValueReferenceType>()->getPointeeType();
   assert(!OtherRefType.getQualifiers() &&
          "Bad argument type of defaulted move assignment");
 
   // Our location for everything implicitly-generated.
-  SourceLocation Loc = MoveAssignOperator->getLocation();
+  SourceLocation Loc = MoveAssignOperator->getLocEnd().isValid()
+                           ? MoveAssignOperator->getLocEnd()
+                           : MoveAssignOperator->getLocation();
 
   // Builds a reference to the "other" object.
   RefBuilder OtherRef(Other, OtherRefType);
   // Cast to rvalue.
   MoveCastBuilder MoveOther(OtherRef);
 
   // Builds the "this" pointer.
   ThisBuilder This;
for (auto *Field : ClassDecl->fields()) {
     }
 
     // Success! Record the copy.
     Statements.push_back(Move.getAs<Stmt>());
   }
 
   if (!Invalid) {
     // Add a "return *this;"
-    ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
-    
+    ExprResult ThisObj =
+        CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
     StmtResult Return = BuildReturnStmt(Loc, ThisObj.get());
     if (Return.isInvalid())
       Invalid = true;
     else {
       Statements.push_back(Return.getAs<Stmt>());
 
       if (Trap.hasErrorOccurred()) {
         Diag(CurrentLocation, diag::note_member_synthesized_at) 
void Sema::DefineImplicitCopyConstructor(SourceLocation CurrentLocation,
   DiagnosticErrorTrap Trap(Diags);
 
   if (SetCtorInitializers(CopyConstructor, /*AnyErrors=*/false) ||
       Trap.hasErrorOccurred()) {
     Diag(CurrentLocation, diag::note_member_synthesized_at) 
       << CXXCopyConstructor << Context.getTagDeclType(ClassDecl);
     CopyConstructor->setInvalidDecl();
   }  else {
+    SourceLocation Loc = CopyConstructor->getLocEnd().isValid()
+                             ? CopyConstructor->getLocEnd()
+                             : CopyConstructor->getLocation();
     Sema::CompoundScopeRAII CompoundScope(*this);
-    CopyConstructor->setBody(ActOnCompoundStmt(
-        CopyConstructor->getLocation(), CopyConstructor->getLocation(), None,
-        /*isStmtExpr=*/ false).getAs<Stmt>());
+    CopyConstructor->setBody(
+        ActOnCompoundStmt(Loc, Loc, None, /*isStmtExpr=*/false).getAs<Stmt>());
   }
 
   CopyConstructor->markUsed(Context);
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(CopyConstructor);
   }
 }
 
void Sema::DefineImplicitMoveConstructor(SourceLocation CurrentLocation,
   DiagnosticErrorTrap Trap(Diags);
 
   if (SetCtorInitializers(MoveConstructor, /*AnyErrors=*/false) ||
       Trap.hasErrorOccurred()) {
     Diag(CurrentLocation, diag::note_member_synthesized_at) 
       << CXXMoveConstructor << Context.getTagDeclType(ClassDecl);
     MoveConstructor->setInvalidDecl();
   }  else {
+    SourceLocation Loc = MoveConstructor->getLocEnd().isValid()
+                             ? MoveConstructor->getLocEnd()
+                             : MoveConstructor->getLocation();
     Sema::CompoundScopeRAII CompoundScope(*this);
     MoveConstructor->setBody(ActOnCompoundStmt(
-        MoveConstructor->getLocation(), MoveConstructor->getLocation(), None,
-        /*isStmtExpr=*/ false).getAs<Stmt>());
+        Loc, Loc, None, /*isStmtExpr=*/ false).getAs<Stmt>());
   }
 
   MoveConstructor->markUsed(Context);
 
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(MoveConstructor);
   }
 }

test/Analysis/inlining/path-notes.cpp

 // CHECK-NEXT:            <key>col</key><integer>18</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:          </array>
 // CHECK-NEXT:         <key>end</key>
 // CHECK-NEXT:          <array>
 // CHECK-NEXT:           <dict>
 // CHECK-NEXT:            <key>line</key><integer>105</integer>
-// CHECK-NEXT:            <key>col</key><integer>21</integer>
+// CHECK-NEXT:            <key>col</key><integer>53</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:           <dict>
 // CHECK-NEXT:            <key>line</key><integer>105</integer>
-// CHECK-NEXT:            <key>col</key><integer>28</integer>
+// CHECK-NEXT:            <key>col</key><integer>53</integer>
 // CHECK-NEXT:            <key>file</key><integer>0</integer>
 // CHECK-NEXT:           </dict>
 // CHECK-NEXT:          </array>
 // CHECK-NEXT:        </dict>
 // CHECK-NEXT:       </array>
 // CHECK-NEXT:     </dict>
 // CHECK-NEXT:     <dict>
 // CHECK-NEXT:      <key>kind</key><string>event</string>
 // CHECK-NEXT:      <key>location</key>
 // CHECK-NEXT:      <dict>
 // CHECK-NEXT:       <key>line</key><integer>105</integer>
-// CHECK-NEXT:       <key>col</key><integer>21</integer>
+// CHECK-NEXT:       <key>col</key><integer>53</integer>
 // CHECK-NEXT:       <key>file</key><integer>0</integer>
 // CHECK-NEXT:      </dict>
 // CHECK-NEXT:      <key>ranges</key>
 // CHECK-NEXT:      <array>
 // CHECK-NEXT:        <array>
 // CHECK-NEXT:         <dict>
 // CHECK-NEXT:          <key>line</key><integer>105</integer>
-// CHECK-NEXT:          <key>col</key><integer>21</integer>
+// CHECK-NEXT:          <key>col</key><integer>53</integer>
 // CHECK-NEXT:          <key>file</key><integer>0</integer>
 // CHECK-NEXT:         </dict>
 // CHECK-NEXT:         <dict>
 // CHECK-NEXT:          <key>line</key><integer>105</integer>
-// CHECK-NEXT:          <key>col</key><integer>28</integer>
+// CHECK-NEXT:          <key>col</key><integer>53</integer>
 // CHECK-NEXT:          <key>file</key><integer>0</integer>
 // CHECK-NEXT:         </dict>
 // CHECK-NEXT:        </array>
 // CHECK-NEXT:      </array>
 // CHECK-NEXT:      <key>depth</key><integer>1</integer>
 // CHECK-NEXT:      <key>extended_message</key>
 // CHECK-NEXT:      <string>Calling move assignment operator for &apos;Dereferencer&apos;</string>
 // CHECK-NEXT:      <key>message</key>

test/Misc/ast-dump-decl.cpp

 // CHECK:        CompoundStmt
 
 class TestCXXDestructorDecl {
   ~TestCXXDestructorDecl() { }
 };
 // CHECK:      CXXDestructorDecl{{.*}} ~TestCXXDestructorDecl 'void (void) noexcept'
 // CHECK-NEXT:   CompoundStmt
 
+// Test that the range of a defaulted members is computed correctly.
+// FIXME: This should include the "= default".
+class TestMemberRanges {
+public:
+  TestMemberRanges() = default;
+  TestMemberRanges(const TestMemberRanges &Other) = default;
+  TestMemberRanges(TestMemberRanges &&Other) = default;
+  ~TestMemberRanges() = default;
+  TestMemberRanges &operator=(const TestMemberRanges &Other) = default;
+  TestMemberRanges &operator=(TestMemberRanges &&Other) = default;
+};
+void SomeFunction() {
+  TestMemberRanges A;
+  TestMemberRanges B(A);
+  B = A;
+  A = static_cast<TestMemberRanges &&>(B);
+  TestMemberRanges C(static_cast<TestMemberRanges &&>(A));
+}
+// CHECK:      CXXConstructorDecl{{.*}} <line:{{.*}}:3, col:20>
+// CHECK:      CXXConstructorDecl{{.*}} <line:{{.*}}:3, col:49>
+// CHECK:      CXXConstructorDecl{{.*}} <line:{{.*}}:3, col:44>
+// CHECK:      CXXDestructorDecl{{.*}} <line:{{.*}}:3, col:21>
+// CHECK:      CXXMethodDecl{{.*}} <line:{{.*}}:3, col:60>
+// CHECK:      CXXMethodDecl{{.*}} <line:{{.*}}:3, col:55>
+
 class TestCXXConversionDecl {
   operator int() { return 0; }
 };
 // CHECK:      CXXConversionDecl{{.*}} operator int 'int (void)'
 // CHECK-NEXT:   CompoundStmt
 
 namespace TestStaticAssertDecl {
   static_assert(true, "msg");

test/SemaTemplate/instantiate-default-assignment-operator.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
 template<typename> struct PassRefPtr { };
 template<typename T> struct RefPtr {
   RefPtr& operator=(const RefPtr&) { int a[sizeof(T) ? -1 : -1];} // expected-error 2 {{array with a negative size}}
   RefPtr& operator=(const PassRefPtr<T>&);
 };
 
 struct A { RefPtr<int> a; };  // expected-note {{instantiation of member function 'RefPtr<int>::operator=' requested here}}
 struct B : RefPtr<float> { }; // expected-note {{in instantiation of member function 'RefPtr<float>::operator=' requested here}}
 
 void f() {
   A a1, a2;
   a1 = a2;
 
   B b1, b2;
-  b1 = b2; 
+  b1 = b2;
 }