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

8883

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::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();
   Destructor->setBody(new (Context) CompoundStmt(Loc));
   Destructor->markUsed(Context);
   MarkVTableUsed(CurrentLocation, ClassDecl);
 
   if (ASTMutationListener *L = getASTMutationListener()) {
     L->CompletedImplicitDefinition(Destructor);
   }
 }

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 destructor is computed correctly.
+// FIXME: This should include the "= default".
+class Base {
+public:
+  virtual ~Base() {}
+};
+class TestDerived : public Base {
+  TestDerived() {}
+  ~TestDerived() = default;
+};
+// CHECK:      CXXDestructorDecl{{.*}} <line:{{.*}}:3, col:16>
+
 class TestCXXConversionDecl {
   operator int() { return 0; }
 };
 // CHECK:      CXXConversionDecl{{.*}} operator int 'int (void)'
 // CHECK-NEXT:   CompoundStmt
 
 namespace TestStaticAssertDecl {
   static_assert(true, "msg");