Safe fallback for known "conflicting overrides" during rename.

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Changes Blink-style names to Chrome-style names. Currently transforms:
 //   fields:
 //     int m_operationCount => int operation_count_
 //   variables (including parameters):
 //     int mySuperVariable => int my_super_variable
 //   constants:
@@ skipping 103 matching lines @@
   if (Node.getNumDecls() == 0)
     return false;
 
   for (clang::NamedDecl* decl : Node.decls()) {
     if (!InnerMatcher.matches(*decl, Finder, Builder))
       return false;
   }
   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,
     T&& inner_matcher,
     clang::ast_matchers::internal::ASTMatchFinder* finder,
     clang::ast_matchers::internal::BoundNodesTreeBuilder* builder) {
   bool override_matches = false;
   bool override_not_matches = false;
 
   for (auto it = decl.begin_overridden_methods();
        it != decl.end_overridden_methods(); ++it) {
     if (MatchAllOverriddenMethods(**it, inner_matcher, finder, builder))
       override_matches = true;
     else
       override_not_matches = true;
   }
 
   // If this fires we have a class overriding a method that matches, and a
   // method that does not match the inner matcher. In that case we will match
   // one ancestor method but not the other. If we rename one of the and not the
   // other it will break what this class overrides, disconnecting it from the
   // 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)
     return true;
 
   return inner_matcher.matches(decl, finder, builder);
 }
 
 AST_MATCHER_P(clang::CXXMethodDecl,
               includeAllOverriddenMethods,
@@ skipping 1244 matching lines @@
   for (const auto& r : replacements) {
     std::string replacement_text = r.getReplacementText().str();
     std::replace(replacement_text.begin(), replacement_text.end(), '\n', '\0');
     llvm::outs() << "r:::" << r.getFilePath() << ":::" << r.getOffset()
                  << ":::" << r.getLength() << ":::" << replacement_text << "\n";
   }
   llvm::outs() << "==== END EDITS ====\n";
 
   return 0;
 }