Safe fallback for known "conflicting overrides" during rename.

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index c3d3356d2542a138cf35bdd3f4e086eb357d4983..09fa7e4d01c239ad4dd4aac5c3765a69015e13f0 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -121,6 +121,13 @@ AST_MATCHER_P(clang::OverloadExpr,
   return true;
 }
 
+void PrintForDiagnostics(clang::raw_ostream& os,
+                         const clang::FunctionDecl& decl) {
+  decl.getLocStart().print(os, decl.getASTContext().getSourceManager());
+  os << ": ";
+  decl.getNameForDiagnostic(os, decl.getASTContext().getPrintingPolicy(), true);
+}
+
 template <typename T>
 bool MatchAllOverriddenMethods(
     const clang::CXXMethodDecl& decl,
@@ -145,13 +152,57 @@ bool MatchAllOverriddenMethods(
   // one we did not rename which creates a behaviour change. So assert and
   // demand the user to fix the code first (or add the method to our
   // blacklist T_T).
-  if (override_matches || override_not_matches)
-    assert(override_matches != override_not_matches);
+  if (override_matches || override_not_matches) {
+    if (override_matches == override_not_matches) {

[danakj, 2017/01/05 18:55:19]

can you change this to &&, it should be the same but more clear

[Łukasz Anforowicz, 2017/01/05 19:04:43]

Good point.  Done.

+      // blink::InternalSettings::trace method overrides
+      // 1) blink::InternalSettingsGenerated::trace
+      //    (won't be renamed because it is in generated code)
+      // 2) blink::Supplement<blink::Page>::trace
+      //    (will be renamed).
+      // It is safe to rename blink::InternalSettings::trace, because
+      // both 1 and 2 will both be renamed (#1 via manual changes of the code
+      // generator for DOM bindings and #2 via the clang tool).
+      // will only rename method overrides

[danakj, 2017/01/05 18:55:19]

this comment isn't a sentence

[Łukasz Anforowicz, 2017/01/05 19:04:44]

Ooops.  Removed this line (leftover from drafting the comment).

+      auto internal_settings_class_decl = cxxRecordDecl(
+          hasName("InternalSettings"),
+          hasParent(namespaceDecl(hasName("blink"),
+                                  hasParent(translationUnitDecl()))));
+      auto is_method_safe_to_rename = cxxMethodDecl(
+          hasName("trace"),
+          anyOf(hasParent(internal_settings_class_decl),  // in .h file
+                has(nestedNameSpecifier(specifiesType(    // in .cpp file
+                    hasDeclaration(internal_settings_class_decl))))));
+      if (IsMatching(is_method_safe_to_rename, decl, decl.getASTContext()))
+        return true;
+
+      // For previously unknown conflicts, errors out and require a human to
+      // analyse the problem (rather than falling back to a potentially unsafe /
+      // code semantics changing rename).
+      llvm::errs() << "ERROR: ";
+      PrintForDiagnostics(llvm::errs(), decl);
+      llvm::errs() << " method overrides "
+                   << "some virtual methods that will be automatically renamed "
+                   << "and some that won't be renamed.";
+      llvm::errs() << "\n";
+      for (auto it = decl.begin_overridden_methods();
+           it != decl.end_overridden_methods(); ++it) {
+        if (MatchAllOverriddenMethods(**it, inner_matcher, finder, builder))
+          llvm::errs() << "Overriden method that will be renamed: ";
+        else
+          llvm::errs() << "Overriden method that will not be renamed: ";
+        PrintForDiagnostics(llvm::errs(), **it);
+        llvm::errs() << "\n";
+      }
+      llvm::errs() << "\n";
+      assert(0);

[Łukasz Anforowicz, 2017/01/05 18:45:19]

We could also return |true| rather than asserting.  I am not sure what is the
right thing here.  Assert seems safer (although potentially disruptive on the
"big day".  WDYT?

[danakj, 2017/01/05 18:55:19]

assert cuz we want to know about it on the big day if it happens!

assert(false) tho

[Łukasz Anforowicz, 2017/01/05 19:04:44]

Done.

+    }
+  }
 
   // If the method overrides something that doesn't match, so the method itself
   // doesn't match.
   if (override_not_matches)
     return false;
+
   // If the method overrides something that matches, so the method ifself
   // matches.
   if (override_matches)