[PATCH] cpp11-migrate: Fix crash in AddOverride due to template instantiations (PR15827)

[Guillaume Papin]

  Fixes according to the review
  Thank you for your answer, my XFAIL example was bad as it matched the
  exact conditions I wanted to avoid adding the 'override' keyword.

Hi revane,

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

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D769?vs=1889&id=1913#toc

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

Index: cpp11-migrate/AddOverride/AddOverrideActions.cpp
===================================================================
--- cpp11-migrate/AddOverride/AddOverrideActions.cpp
+++ cpp11-migrate/AddOverride/AddOverrideActions.cpp
@@ -32,28 +32,40 @@
   const CXXMethodDecl *M = Result.Nodes.getDeclAs<CXXMethodDecl>(MethodId);
   assert(M && "Bad Callback. No node provided");
 
+  if (!SM.isFromMainFile(M->getLocStart()))
+    return;
+
   // First check that there isn't already an override attribute.
-  if (!M->hasAttr<OverrideAttr>()) {
-    if (M->getLocStart().isFileID()) {
-      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);
-      } else {
-        StartLoc = SM.getSpellingLoc(M->getLocEnd());
-        StartLoc = Lexer::getLocForEndOfToken(StartLoc, 0, SM, LangOptions());
-      }
-      Replace.insert(tooling::Replacement(SM, StartLoc, 0, " override"));
-      ++AcceptedChanges;
-    }
+  if (M->hasAttr<OverrideAttr>())
+    return;
+
+  // FIXME: Pure methods are not supported yet as it is difficult to track down
+  // the location of '= 0'.
+  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);
+  } else {
+    StartLoc = SM.getSpellingLoc(M->getLocEnd());
+    StartLoc = Lexer::getLocForEndOfToken(StartLoc, 0, SM, LangOptions());
   }
+  Replace.insert(tooling::Replacement(SM, StartLoc, 0, " override"));
+  ++AcceptedChanges;
 }
-
Index: test/cpp11-migrate/AddOverride/basic.cpp
===================================================================
--- test/cpp11-migrate/AddOverride/basic.cpp
+++ test/cpp11-migrate/AddOverride/basic.cpp
@@ -108,3 +108,41 @@
   // CHECK: void h() const LLVM_OVERRIDE;
 };
 
+// Test that override isn't added at the wrong place for "pure overrides"
+struct APure {
+  virtual APure *clone() = 0;
+};
+struct BPure : APure {
+  virtual BPure *clone() { return new BPure(); }
+};
+struct CPure : BPure {
+  virtual BPure *clone() = 0;
+  // CHECK: struct CPure : BPure {
+  // CHECK-NOT: virtual BPure *clone() = 0 override;
+  // CHECK: };
+};
+struct DPure : CPure {
+  virtual DPure *clone() { return new DPure(); }
+};
+
+// Test that override is not added on dangerous template constructs
+struct Base1 {
+  virtual void f();
+};
+struct Base2 {};
+template<typename T> struct Derived : T {
+  void f(); // adding 'override' here will break instantiation of Derived<Base2>
+  // CHECK: struct Derived
+  // CHECK: void f();
+};
+Derived<Base1> d1;
+Derived<Base2> d2;
+
+template <typename T>
+class M : public A {
+public:
+  virtual void i();
+  // CHECK: class M : public A {
+  // CHECK: virtual void i() override;
+};
+M<int> b;
Index: test/cpp11-migrate/AddOverride/pure_specifier_fail.cpp
===================================================================
--- /dev/null
+++ test/cpp11-migrate/AddOverride/pure_specifier_fail.cpp
@@ -0,0 +1,20 @@
+// 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: *
+
+// Test that override isn't placed correctly after "pure overrides"
+struct A {
+  virtual A *clone() = 0;
+};
+struct B : A {
+  virtual B *clone() { return new B(); }
+};
+struct C : B {
+  virtual B *clone() = 0;
+  // CHECK: struct C : B {
+  // CHECK: virtual B *clone() override = 0;
+};
+struct D : C {
+  virtual D *clone() { return new D(); }
+};
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D769.2.patch
Type: text/x-patch
Size: 4690 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130509/54c5ceb1/attachment.bin>