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

[Guillaume Papin]

Hi revane,

This commit fixes a "FIXME" in the add-override transform. ' override'
was misplaced when a comment was between the function body and the end
of the 'prototype'.

It also remove duplicated check for the main file from the last commit
(and fixes the typo in the comment above).

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

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,36 @@
 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;
-
+  // Check that the method declaration is in the main file
   if (!SM.isFromMainFile(M->getLocStart()))
     return;
 
@@ -56,16 +76,9 @@
 
   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.1.patch
Type: text/x-patch
Size: 3981 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130511/bc298c3e/attachment.bin>