Skip parameter replacements where old text != old name.

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 505 matching lines @@
     std::string new_name;
     if (!GetNameForDecl(*decl, *context, new_name))
       return;  // If false, the name was not suitable for renaming.
     llvm::StringRef old_name = decl->getName();
     if (old_name == new_name)
       return;
     clang::SourceLocation loc = TargetNodeTraits<TargetNode>::GetLoc(
         *result.Nodes.getNodeAs<TargetNode>(
             TargetNodeTraits<TargetNode>::GetName()));
     clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc);
+
+    // Check if |range| really covers text of |old_name|.  This might not be
+    // true when specializing a templatized class that had an unnamed parameter
+    // in one of method declarations.
+    StringRef old_text = clang::Lexer::getSourceText(
+        range, *result.SourceManager, result.Context->getLangOpts());
+    if (!loc.isMacroID() && old_text != old_name)
+      return;
+

[Łukasz Anforowicz, 2016/08/15 23:05:08]

The new code makes the new regression test pass.  This is good I guess.  OTOH,
the new code feels like papering over some other mistakes (that I cannot quite
put my finger on):

- How on earth things worked before, even if old_text != old_name?

- I don't understand how we get into a situation where old_text != old_name.  I
am guessing that |loc| is not always the location of |decl->getName()|.  I don't
understand the significance of 1) passing "decl" (from
TargetNodeTraits<clang::NamedDecl>::GetName right?) and 2) using
TargetNode=clang::NamedDecl (instead of clang::VarDecl) when calling
clang::ast_matchers::BoundNodes::getNodeAs<TargetNode>


Note about macros
=================

Removing |!loc.isMacroID()| condition in the fix will introduce some regressions
in rewrite_to_chrome_style/tests/macros-original.cc:

-  inline long long ToInt(argumentType* argumentName) {                     \
+  inline long long toInt(argumentType* argumentName) {                     \

-  if (LIKELY(derived_ptr)) {
+  if (LIKELY(derivedPtr)) {

 #define CALL_METHOD_FROM_MACRO()           \
-  void CallMethodFromMacro() { Method(); } \
-  void Pmethod() override {}
+  void callMethodFromMacro() { method(); } \
+  void pmethod() override {}

I was surprised to find out that the tool used to perform the right replacements
even though sometimes old_text != old_name as shown below:

Printf-style debugging from above when processing macros-actual.cc:

DEBUG; old_text=''; old_name='toInt'; new_name='ToInt'
DEBUG; old_text='LIKELY(derivedPtr)'; old_name='derivedPtr';
new_name='derived_ptr'
DEBUG; old_text=''; old_name='callMethodFromMacro';
new_name='CallMethodFromMacro'
DEBUG; old_text=''; old_name='method'; new_name='Method'
DEBUG; old_text=''; old_name='pmethod'; new_name='Pmethod'

...

[dcheng, 2016/08/25 04:08:51]

How were you printing out this diagnostic info? Replacement uses the "spelling
location":
http://clang.llvm.org/doxygen/classclang_1_1SourceManager.html#a989650d5a9a5d...

So it's possible that what Replacement is doing and the debugging code were
doing didn't quite match.

[Łukasz Anforowicz, 2016/08/25 23:32:33]

Thanks for bringing up "spelling location".  I can no longer repro the mismatch
after changing RewriterBase::run to end with:

    clang::SourceLocation loc2 =
context->getSourceManager().getSpellingLoc(loc);
    clang::CharSourceRange range2 = clang::CharSourceRange::getTokenRange(loc2);
    StringRef old_text = clang::Lexer::getSourceText(
        range2, context->getSourceManager(), context->getLangOpts());
    assert(old_text == old_name);

I would hit the assert if using loc2 = loc.

So:
- In the new code above, I need to use a spelling location.
- I don't need loc.IsMacroID

PS. I assume it is okay for the current code of RewriterBase::run to use the
current (non-spelling) location, because Replacements will take care of
translating |loc| into an equivalent spelling location.

     replacements_->emplace(*result.SourceManager, range, new_name);
     replacement_names_.emplace(old_name.str(), std::move(new_name));
   }
 
   const std::unordered_map<std::string, std::string>& replacement_names()
       const {
     return replacement_names_;
   }
 
  private:
@@ skipping 344 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;
 }