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

[Guillaume Papin]

  Address the comments from the review. Update the AddOverride
  documentation as the known limitations have changed recently.

Hi revane,

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

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

Files:
  cpp11-migrate/AddOverride/AddOverrideActions.cpp
  docs/AddOverrideTransform.rst
  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,41 @@
 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());
+    const bool Invalid =
+        Lexer::getRawToken(Beginning, Tok, SM, Context.getLangOpts());
+
+    assert(!Invalid && "Expected a valid token.");
+    if (Invalid || Tok.getKind() != tok::comment)
+      return Loc.getLocWithOffset(1);
+  }
+}
+
+} // end anonymous namespace
+
 void AddOverrideFixer::run(const MatchFinder::MatchResult &Result) {
   SourceManager &SM = *Result.SourceManager;
 
   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 +73,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, *Result.Context,
+                                                  M->getBody()->getLocStart());
   } else {
     StartLoc = SM.getSpellingLoc(M->getLocEnd());
     StartLoc = Lexer::getLocForEndOfToken(StartLoc, 0, SM, LangOptions());
Index: docs/AddOverrideTransform.rst
===================================================================
--- docs/AddOverrideTransform.rst
+++ docs/AddOverrideTransform.rst
@@ -28,20 +28,17 @@
 
 Known Limitations
 -----------------
-* This transform will fail if a method declaration has an inlined method
-  body and there is a comment between the method declaration and the body.
-  In this case, the override keyword will incorrectly be inserted at the 
-  end of the comment.
+* This transform will not insert the override keyword if a method is
+  pure. At the moment it's not possible to track down the pure
+  specifier location.
 
 .. code-block:: c++
 
   class B : public A {
   public:
-    virtual void h() const // comment
-    { }
+    virtual void h() const = 0;
 
-    // The declaration of h is transformed to
-    virtual void h() const // comment override
-    { }
+    // The declaration of h is NOT transformed to
+    virtual void h() const override = 0;
   };
 
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.3.patch
Type: text/x-patch
Size: 5370 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130513/01f41e85/attachment.bin>