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 ↗

(On Diff #22368)

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

80 ↗

(On Diff #22368)

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 ↗

(On Diff #22368)

Great suggestion, thanks. Done

80 ↗

(On Diff #22368)

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

cfe/trunk/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;
+    SourceLocation KWEndLoc = Tok.getEndLoc().getLocWithOffset(-1);
     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)) {
+        DeclAsFunction->setRangeEnd(KWEndLoc);
+      }
     } 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)) {
+        DeclAsFunction->setRangeEnd(KWEndLoc);
+      }
     } 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);

cfe/trunk/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>

cfe/trunk/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 {

cfe/trunk/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()));
 }