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

[Edwin Vane]

  * The isMainFile() check seems to be missing.

  * The call to isDependentContext() basically causes all template classes to be ignored, even really safe ones. For example, the following test fails:

    struct A {
    virtual void foo();
  };
  template <typename T>
  struct B : public A {
    virtual void foo();
    // CHECK: struct B : public A {
    // CHECK: virtual void foo() override;
  };
  B<int> b;

  If this was your intent, I'd add this test to the non_risky_... XFAIL test.

  * Could you log enhancements in bugzilla (the public C++11 Migrator JIRA instance isn't quite ready) about fixing the two XFAIL tests and then mention the PR numbers in the test comments?


================
Comment at: cpp11-migrate/AddOverride/AddOverrideActions.cpp:37
@@ +36,3 @@
+  if (M->hasAttr<OverrideAttr>())
+    return ;
+
----------------
No spaces between return and ;. I recommend using the clang-format tool on your changes and it'll handle all the style stuff for you.

================
Comment at: cpp11-migrate/AddOverride/AddOverrideActions.cpp:57
@@ +56,3 @@
+  const FunctionDecl *Fn;
+  if (M->hasBody(Fn) && !Fn->isOutOfLine()) {
+    // Start at the beginning of the body and rewind back to the last
----------------
I'd recommend still using hasInlineBody() as it's clearer as to what this code is for. What you have here is identical to the contents of that other function except for the getTemplateInstantiationPattern() which should return NULL now since you've updated what M is.

================
Comment at: cpp11-migrate/AddOverride/AddOverrideActions.cpp:52
@@ +51,3 @@
+  if (M->isDependentContext()) {
+    return ;
+  }
----------------
Style says no braces for single-line if control statements.

================
Comment at: cpp11-migrate/AddOverride/AddOverrideActions.cpp:46
@@ -56,1 +45,3 @@
+    if (const CXXMethodDecl *TemplateMethod = dyn_cast<CXXMethodDecl>(F)) {
+      M = TemplateMethod;
     }
----------------
I think you can drop both sets of  braces here without impacting readability.

================
Comment at: cpp11-migrate/AddOverride/AddOverrideActions.cpp:45
@@ +44,3 @@
+  if (const FunctionDecl *F = M->getTemplateInstantiationPattern()) {
+    if (const CXXMethodDecl *TemplateMethod = dyn_cast<CXXMethodDecl>(F)) {
+      M = TemplateMethod;
----------------
I think maybe I'd use cast<> here instead to cause an assertion failure if the type is not CXXMethodDecl as I can't think of a situation where this should not be the case.


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