Fix source range of defaulted/deleted member declarations

[eliben]

Summary

Fixes https://llvm.org/bugs/show_bug.cgi?id=20744

struct A {

A() = default;

};

Previously the source range of the declaration of A ended at the ')'. It should include the '= default' part as well. The same for '= delete'.

Note: this will break one of the clang-tidy fixers, which is going to be addessed in a follow-up patch.

[eliben]

Note: followup patch to clang-tidy is http://reviews.llvm.org/D8466

[jpienaar]

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

[eliben]

In D8465#144139, @jpienaar wrote:

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

The problem with ParseCXXInlineMethodDef is that we no longer have access to these tokens in it. ParseCXXInlineMethodDef expects to see a { for a def or ; for a =default/=delete method. So we'd have to push DeleteOrDefaultEndLoc through to ParseCXXInlineMethodDef - it would result in an additional argument, more code and I'm not sure what gain.

[jpienaar]

In D8465#144141, @eliben wrote:

In D8465#144139, @jpienaar wrote:

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

The problem with ParseCXXInlineMethodDef is that we no longer have access to these tokens in it. ParseCXXInlineMethodDef expects to see a { for a def or ; for a =default/=delete method. So we'd have to push DeleteOrDefaultEndLoc through to ParseCXXInlineMethodDef - it would result in an additional argument, more code and I'm not sure what gain.

OK, I thought KWLoc that is set in ParseCXXInlineMethodDef could be used to get it.

[rnk]

It doesn't look too hard to do this locally in ParseCXXInlineMethodDef by saving the end of the current token location before calling TryConsumeToken:

SourceLocation KWEndLoc = Tok.getEndLoc();
if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
} else if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
}

Seem reasonable?

[eliben]

In D8465#144236, @rnk wrote:

It doesn't look too hard to do this locally in ParseCXXInlineMethodDef by saving the end of the current token location before calling TryConsumeToken:

SourceLocation KWEndLoc = Tok.getEndLoc();
if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
} else if (TryConsumeToken(...)) {
  ...
  FnD->setRangeEnd(KWEndLoc);
}

Seem reasonable?

Ah, indeed. This makes things simpler. PTAL

Thanks

[eliben]

In D8465#144183, @jpienaar wrote:

In D8465#144141, @eliben wrote:

In D8465#144139, @jpienaar wrote:

Looks good. Is it better to adjust the range here or n ParseCXXInlineMethodDef?

The problem with ParseCXXInlineMethodDef is that we no longer have access to these tokens in it. ParseCXXInlineMethodDef expects to see a { for a def or ; for a =default/=delete method. So we'd have to push DeleteOrDefaultEndLoc through to ParseCXXInlineMethodDef - it would result in an additional argument, more code and I'm not sure what gain.

OK, I thought KWLoc that is set in ParseCXXInlineMethodDef could be used to get it.

Yes, indeed. Updated the patch.

Thanks

[eliben]

whitespace cleanup

[eliben]

Reid, PTAL

[rnk]

lib/Parse/ParseCXXInlineMethods.cpp

73

Any reason we can't save Tok.getEndLoc() here? Then we don't have to hardcode the lengths of delete and default.

80

This doesn't do the right thing for function templates, which the only case where this dyn_cast can fail. I think you can write a templated constructor and default it. =P A robust way to do this might be:

auto *FD = dyn_cast<FunctionDecl>(FnD);
FD = FD ? FD : cast<FunctionTemplateDecl>(FnD)->getTemplatedDecl();
FD->setRangeEnd(KWEndLoc);

[eliben]

lib/Parse/ParseCXXInlineMethods.cpp

73

Great suggestion, thanks. Done

80

Reid, I may be missing something here, do you mean something like:

struct Foot {

template <typename T>
Foot &operator=(const T& Other) = default;

};

I don't think this is actually valid. The standard says in 8.4.2:

"A function that is explicitly defaulted shall ... have the same declared function type (except for possibly differing ref-qualifiers and except that in the case of a copy constructor or copy assignment operator, the parameter type may be “reference to non-const T”, where T is the name of the member function’s class) as if it had been implicitly declared,
and"

And indeed Clang complains:

error: only special member functions may be defaulted
   Foot &operator=(const T& Other) = default;

[eliben]

Using Tok.getEndLoc(). to avoid recomputing

[rnk]

lgtm

[eliben]

r233028

Thanks for the review

lib/Parse/ParseCXXInlineMethods.cpp

NamedDecl *Parser::ParseCXXInlineMethodDef(AccessSpecifier AS,
 
   if (TryConsumeToken(tok::equal)) {
     if (!FnD) {
       SkipUntil(tok::semi);
       return nullptr;
     }
 
     bool Delete = false;
     SourceLocation KWLoc;
     if (TryConsumeToken(tok::kw_delete, KWLoc)) {
       Diag(KWLoc, getLangOpts().CPlusPlus11
                       ? diag::warn_cxx98_compat_deleted_function
                       : diag::ext_deleted_function);
       Actions.SetDeclDeleted(FnD, KWLoc);
       Delete = true;
+      if (auto *DeclAsFunction = dyn_cast<FunctionDecl>(FnD)) {
+        SourceLocation DeleteEndLoc = KWLoc.getLocWithOffset(5);
+        DeclAsFunction->setRangeEnd(DeleteEndLoc);
+      }
     } else if (TryConsumeToken(tok::kw_default, KWLoc)) {
       Diag(KWLoc, getLangOpts().CPlusPlus11
                       ? diag::warn_cxx98_compat_defaulted_function
                       : diag::ext_defaulted_function);
       Actions.SetDeclDefaulted(FnD, KWLoc);
+      if (auto *DeclAsFunction = dyn_cast<FunctionDecl>(FnD)) {
+        SourceLocation DefaultEndLoc = KWLoc.getLocWithOffset(6);
+        DeclAsFunction->setRangeEnd(DefaultEndLoc);
+      }
     } else {
       llvm_unreachable("function definition after = not 'delete' or 'default'");
     }
 
     if (Tok.is(tok::comma)) {
       Diag(KWLoc, diag::err_default_delete_in_multiple_declaration)
         << Delete;
       SkipUntil(tok::semi);

lib/Parse/ParseDeclCXX.cpp

       Init = ParseInitializer();
       if (Init.isInvalid())
         SkipUntil(tok::comma, StopAtSemi | StopBeforeMatch);
       else
         HasInitializer = true;
     }
 
     FunctionDefinitionKind DefinitionKind = FDK_Declaration;
+
     // function-definition:
     //
     // In C++11, a non-function declarator followed by an open brace is a
     // braced-init-list for an in-class member initialization, not an
     // erroneous function definition.
     if (Tok.is(tok::l_brace) && !getLangOpts().CPlusPlus11) {
       DefinitionKind = FDK_Definition;
     } else if (DeclaratorInfo.isFunctionDeclarator()) {

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>53</integer>
+// CHECK-NEXT:            <key>col</key><integer>63</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>53</integer>
+// CHECK-NEXT:            <key>col</key><integer>63</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>53</integer>
+// CHECK-NEXT:       <key>col</key><integer>63</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>53</integer>
+// CHECK-NEXT:          <key>col</key><integer>63</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>53</integer>
+// CHECK-NEXT:          <key>col</key><integer>63</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

 
 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>
+// CHECK:      CXXConstructorDecl{{.*}} <line:{{.*}}:3, col:30>
+// CHECK:      CXXConstructorDecl{{.*}} <line:{{.*}}:3, col:59>
+// CHECK:      CXXConstructorDecl{{.*}} <line:{{.*}}:3, col:54>
+// CHECK:      CXXDestructorDecl{{.*}} <line:{{.*}}:3, col:31>
+// CHECK:      CXXMethodDecl{{.*}} <line:{{.*}}:3, col:70>
+// CHECK:      CXXMethodDecl{{.*}} <line:{{.*}}:3, col:65>
 
 class TestCXXConversionDecl {
   operator int() { return 0; }
 };
 // CHECK:      CXXConversionDecl{{.*}} operator int 'int (void)'
 // CHECK-NEXT:   CompoundStmt
 
 namespace TestStaticAssertDecl {

unittests/AST/SourceLocationTest.cpp

 }
 
 TEST(CXXConstructorDecl, NoRetFunTypeLocRange) {
   RangeVerifier<CXXConstructorDecl> Verifier;
   Verifier.expectRange(1, 11, 1, 13);
   EXPECT_TRUE(Verifier.match("class C { C(); };", functionDecl()));
 }
 
+TEST(CXXConstructorDecl, DefaultedCtorLocRange) {
+  RangeVerifier<CXXConstructorDecl> Verifier;
+  Verifier.expectRange(1, 11, 1, 23);
+  EXPECT_TRUE(Verifier.match("class C { C() = default; };", functionDecl()));
+}
+
+TEST(CXXConstructorDecl, DeletedCtorLocRange) {
+  RangeVerifier<CXXConstructorDecl> Verifier;
+  Verifier.expectRange(1, 11, 1, 22);
+  EXPECT_TRUE(Verifier.match("class C { C() = delete; };", functionDecl()));
+}
+
 TEST(CompoundLiteralExpr, CompoundVectorLiteralRange) {
   RangeVerifier<CompoundLiteralExpr> Verifier;
   Verifier.expectRange(2, 11, 2, 22);
   EXPECT_TRUE(Verifier.match(
                   "typedef int int2 __attribute__((ext_vector_type(2)));\n"
                   "int2 i2 = (int2){1, 2};", compoundLiteralExpr()));
 }