Skip parameter replacements where old text != old name.

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 9ae3201a4c2b0964141b30d3207de19373c716b6..e650e32bfc2baff67b14d094da0b0e73c64446a9 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -523,6 +523,15 @@ class RewriterBase : public MatchFinder::MatchCallback {
         *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));
   }