Support rewriting |to##macroArg()| into |To##macroArg()|.

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 e142dc2639c45afd7f2c4de72e8e0d8feb3899b8..b60bf670fab8864bdf35339719881931d605ded1 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -692,8 +692,47 @@ class RewriterBase : public MatchFinder::MatchCallback {
 
     clang::SourceLocation loc =
         TargetNodeTraits<TargetNode>::GetLoc(GetTargetNode(result));
-    clang::CharSourceRange range = clang::CharSourceRange::getTokenRange(loc);
-    replacements_->emplace(*result.SourceManager, range, new_name);
+
+    if (loc.isMacroID()) {
+      // Try to jump "above" the scratch buffer if |loc| is inside
+      // token##Concatenation.
+      const int kMaxJumps = 5;
+      bool verified_out_of_scratch_space = false;
+      for (int i = 0; i < kMaxJumps && !verified_out_of_scratch_space; i++) {
+        clang::SourceLocation spell = result.SourceManager->getSpellingLoc(loc);
+        verified_out_of_scratch_space =
+            result.SourceManager->getBufferName(spell) != "<scratch space>";
+        if (!verified_out_of_scratch_space)
+          loc = result.SourceManager->getImmediateMacroCallerLoc(loc);
+      }
+      if (!verified_out_of_scratch_space)

[dcheng, 2016/12/22 11:35:04]

It turns out I implemented something similar, but in a slightly different way.

[Łukasz Anforowicz, 2016/12/22 17:59:30]

Acknowledged.  I considered switching to your way (which does look simpler /
better) but you are doing something slightly different - I don't think your
conditions would work here.

+        return;
+    }
+
+    // Minimize the diff - this is important for dealing with toFooBar ->

[dcheng, 2016/12/22 11:35:04]

Perhaps mention that if the edit only affect 1 character (e.g. probably a method
rename), then we attempt to collapse the edit to accomodate macros that use
token pasting to declare methods.

(From the comment, I expected this to be a generic minimize routine)

[Łukasz Anforowicz, 2016/12/22 17:59:30]

Ok - done.
It was meant to be a generic minimize routine eventually :-P  But probably not
worth to add Foo -> k##Foo processing, so you are right that we should have the
comment document what is actually implemented.

+    // ToFooBar method renaming when the method name is built using macro
+    // token concatenation like to##macroArgument - in this case we should
+    // only rewrite "t" -> "T" and leave "o##macroArgument" untouched.
+    llvm::StringRef expected_old_text = old_name;
+    llvm::StringRef new_text = new_name;
+    if (expected_old_text.substr(1) == new_text.substr(1)) {
+      expected_old_text = expected_old_text.substr(0, 1);
+      new_text = new_text.substr(0, 1);
+    }
+    clang::SourceLocation spell = result.SourceManager->getSpellingLoc(loc);
+    clang::CharSourceRange range = clang::CharSourceRange::getCharRange(
+        spell, spell.getLocWithOffset(expected_old_text.size()));

[dcheng, 2016/12/22 11:35:04]

It's probably worth comparing the entire original argument to be safe, right?

[Łukasz Anforowicz, 2016/12/22 17:59:30]

Comparing the entire |old_name| is wrong - it will be different in case of
to##macroArgument.

I think we have 2 options:

1. Do nothing:
   - works fine for all known cases, including to##macroArgument
   - will not help workaround the anonymous-parameter-kind-of-screw-up in case
the edit is minimized to 1 character only.  but there are no known cases like
this (i.e. param edits won't minimize to 1 character edit)

2. Only minimize if we actually *were* in a macro.


I think #1 is a fine option.  WDYT?

[dcheng, 2016/12/22 21:32:41]

I don't think doing this hurts, so we may as well keep it for now? I'd be OK
either way though.

[Łukasz Anforowicz, 2016/12/23 00:17:48]

I've decided to avoid minimization:
1) it keeps the tool output semi-human-readable
2) it was easier to do than I thought (just check for loc.isMacroID() before
minimization).

+
+    // We need to ensure that |actual_old_text| is the same as
+    // |expected_old_text| - it can be different if |actual_old_text| represents
+    // a macro argument (see DEFINE_WITH_TOKEN_CONCATENATION2 in
+    // macros-original.cc testcase).
+    StringRef actual_old_text = clang::Lexer::getSourceText(
+        range, *result.SourceManager, result.Context->getLangOpts());
+    if (actual_old_text != expected_old_text)
+      return;
+
+    replacements_->emplace(*result.SourceManager, range, new_text);
     replacement_names_.emplace(old_name.str(), std::move(new_name));
   }
 
@@ -721,18 +760,6 @@ class DeclRewriterBase : public RewriterBase<TargetNode> {
     // If false, there's no name to be renamed.
     if (!decl->getIdentifier())
       return;
-    clang::SourceLocation decl_loc =
-        TargetNodeTraits<clang::NamedDecl>::GetLoc(*decl);
-    if (decl_loc.isMacroID()) {
-      // Get the location of the spelling of the declaration. If token pasting
-      // was used this will be in "scratch space" and we don't know how to get
-      // from there back to/ the actual macro with the foo##bar text. So just
-      // don't replace in that case.
-      clang::SourceLocation spell =
-          result.SourceManager->getSpellingLoc(decl_loc);
-      if (result.SourceManager->getBufferName(spell) == "<scratch space>")
-        return;
-    }
     clang::ASTContext* context = result.Context;
     std::string new_name;
     if (!GetNameForDecl(*decl, *context, new_name))