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

[Guillaume Papin]

Hi revane,

This patch fixes different issues:
  - override is not added in template 'contexts' (this will be further improved to handle safe situations, a test for this has been written already)
  - the main file is now checked before the modifications are applied
  - override is not applied now when dealing with pure methods since it was misplaced (ignoring it isn't the perfect solution but it seems difficult to find the location just before the pure-specifier)

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

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

Index: cpp11-migrate/AddOverride/AddOverrideActions.cpp
===================================================================
--- cpp11-migrate/AddOverride/AddOverrideActions.cpp
+++ cpp11-migrate/AddOverride/AddOverrideActions.cpp
@@ -33,27 +33,42 @@
   assert(M && "Bad Callback. No node provided");
 
   // 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 *F = M->getTemplateInstantiationPattern()) {
+    if (const CXXMethodDecl *TemplateMethod = dyn_cast<CXXMethodDecl>(F)) {
+      M = TemplateMethod;
     }
   }
-}
 
+  // FIXME: Templates should be accepted if not in base-specifier.
+  if (M->isDependentContext()) {
+    return ;
+  }
+
+  SourceLocation StartLoc;
+  const FunctionDecl *Fn;
+  if (M->hasBody(Fn) && !Fn->isOutOfLine()) {
+    // 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,32 @@
   // 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;
Index: test/cpp11-migrate/AddOverride/non_risky_template_instantiation.cpp
===================================================================
--- /dev/null
+++ test/cpp11-migrate/AddOverride/non_risky_template_instantiation.cpp
@@ -0,0 +1,23 @@
+// 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 is placed correctly if there is a body and the
+// function was instantiated from a 'non-risky' template
+template<typename T>
+struct M
+{
+  virtual ~M();
+  virtual T f();
+};
+
+template<typename T>
+struct N : public M<T>
+{
+  virtual T f() { }
+  // CHECK: struct N
+  // CHECK: T f() override { }
+};
+
+extern template class N<char>;
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D769.1.patch
Type: text/x-patch
Size: 4583 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130508/e5db31d3/attachment.bin>