[PATCH] cpp11-migrate: Add override specifier before comments on inline methods

[Guillaume Papin]

  Move the template instantiation code before the check for the main
  file, as they have no guarantee to be in the same file.

Hi revane,

http://llvm-reviews.chandlerc.com/D782

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D782?vs=1928&id=1929#toc

Files:
  cpp11-migrate/AddOverride/AddOverrideActions.cpp
  test/cpp11-migrate/AddOverride/basic.cpp
  test/cpp11-migrate/AddOverride/comment_before_inline_body_fail.cpp

Index: cpp11-migrate/AddOverride/AddOverrideActions.cpp
===================================================================
--- cpp11-migrate/AddOverride/AddOverrideActions.cpp
+++ cpp11-migrate/AddOverride/AddOverrideActions.cpp
@@ -26,16 +26,39 @@
 using namespace clang::tooling;
 using namespace clang;
 
+namespace {
+
+SourceLocation
+backwardSkipWhitespacesAndComments(const SourceManager &SM,
+                                   const clang::ASTContext &Context,
+                                   SourceLocation Loc) {
+  for (;;) {
+    do {
+      Loc = Loc.getLocWithOffset(-1);
+    } while (isWhitespace(*FullSourceLoc(Loc, SM).getCharacterData()));
+
+    Token Tok;
+    SourceLocation Beginning =
+        Lexer::GetBeginningOfToken(Loc, SM, Context.getLangOpts());
+    if (Lexer::getRawToken(Beginning, Tok, SM, Context.getLangOpts()) ||
+        Tok.getKind() != tok::comment)
+      return Loc.getLocWithOffset(1);
+  }
+}
+
+} // end anonymous namespace
+
 void AddOverrideFixer::run(const MatchFinder::MatchResult &Result) {
   SourceManager &SM = *Result.SourceManager;
+  const ASTContext &Context = *Result.Context;
 
   const CXXMethodDecl *M = Result.Nodes.getDeclAs<CXXMethodDecl>(MethodId);
   assert(M && "Bad Callback. No node provided");
 
-  // Check that the method declaration in the main file
-  if (!SM.isFromMainFile(M->getLocStart()))
-    return;
+  if (const FunctionDecl *TemplateMethod = M->getTemplateInstantiationPattern())
+    M = cast<CXXMethodDecl>(TemplateMethod);
 
+  // Check that the method declaration is in the main file
   if (!SM.isFromMainFile(M->getLocStart()))
     return;
 
@@ -48,24 +71,14 @@
   if (M->isPure())
     return;
 
-  if (const FunctionDecl *TemplateMethod = M->getTemplateInstantiationPattern())
-    M = cast<CXXMethodDecl>(TemplateMethod);
-
   if (M->getParent()->hasAnyDependentBases())
     return;
 
   SourceLocation StartLoc;
   if (M->hasInlineBody()) {
-    // Start at the beginning of the body and rewind back to the last
-    // non-whitespace character. We will insert the override keyword
-    // after that character.
-    // FIXME: This transform won't work if there is a comment between
-    // the end of the function prototype and the start of the body.
-    StartLoc = M->getBody()->getLocStart();
-    do {
-      StartLoc = StartLoc.getLocWithOffset(-1);
-    } while (isWhitespace(*FullSourceLoc(StartLoc, SM).getCharacterData()));
-    StartLoc = StartLoc.getLocWithOffset(1);
+    // Insert the override specifier before the function body.
+    StartLoc = backwardSkipWhitespacesAndComments(SM, Context,
+                                                  M->getBody()->getLocStart());
   } else {
     StartLoc = SM.getSpellingLoc(M->getLocEnd());
     StartLoc = Lexer::getLocForEndOfToken(StartLoc, 0, SM, LangOptions());
Index: test/cpp11-migrate/AddOverride/basic.cpp
===================================================================
--- test/cpp11-migrate/AddOverride/basic.cpp
+++ test/cpp11-migrate/AddOverride/basic.cpp
@@ -57,8 +57,12 @@
 class G : public A {
 public:
   void h() const; // comment
+  void i() // comment
+  {}
   // CHECK: class G
   // CHECK: void h() const override; // comment
+  // CHECK: void i() override // comment
+  // CHECK-NEXT: {}
 };
 
 // Test that override is placed correctly if there is an inline body.
Index: test/cpp11-migrate/AddOverride/comment_before_inline_body_fail.cpp
===================================================================
--- test/cpp11-migrate/AddOverride/comment_before_inline_body_fail.cpp
+++ /dev/null
@@ -1,25 +0,0 @@
-// RUN: grep -Ev "// *[A-Z-]+:" %s > %t.cpp
-// RUN: cpp11-migrate -add-override %t.cpp -- -I %S
-// RUN: FileCheck -input-file=%t.cpp %s
-// XFAIL: *
-
-class A {
-public:
-  virtual void h() const;
-  // CHECK: virtual void h() const;
-};
-
-// Test that the override is correctly placed if there
-// is an inline comment between the function declaration
-// and the function body.
-// This test fails with the override keyword being added
-// to the end of the comment. This failure occurs because
-// the insertion point is incorrectly calculated if there
-// is an inline comment before the method body.
-class B : public A {
-public:
-  virtual void h() const // comment
-  { }
-  // CHECK: virtual void h() const override
-};
-
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D782.2.patch
Type: text/x-patch
Size: 4335 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130511/9f3ace93/attachment.bin>