Skip parameter replacements where old text != old name.

Skip ParmVarDecl nodes without a name.

BUG=598141
Committed: https://crrev.com/1333f1b925e384fe418c6ea9205b0383bd73f396
Cr-Commit-Position: refs/heads/master@{#414877}

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

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

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

Daniel, can you take a look please?  Honest feedback please - it feels as if I
am trying random things and seeing what sticks... :-(

https://codereview.chromium.org/2246263002/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:534: 
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'

...

[Łukasz Anforowicz, 2016-08-23 20:36:15]

Description was changed from

==========
Skip replacements where old text != old name.

BUG=598141
==========

to

==========
Skip ParmVarDecl nodes without a name.

BUG=598141
==========

[Łukasz Anforowicz, 2016-08-23 20:46:10]

Daniel, can you take a look please?

I think this CL is ready for a review - after applying all the 3 CLs
below, and running the tool on Source/wtf, things start to compile
without any errors (so far I've tested building of PrintStream.o).
This increases my confidence that the 3 CLs below really are moving
things in the right direction:
- https://codereview.chromium.org/2246263002
- https://codereview.chromium.org/2274653002
- https://codereview.chromium.org/2256913002

[dcheng, 2016-08-25 04:08:52]

https://codereview.chromium.org/2246263002/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:534: 
On 2016/08/15 23:05:08, Łukasz Anforowicz wrote:
> 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'
> 
> ...

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.

https://codereview.chromium.org/2246263002/diff/60001/docs/clang_tool_refacto...
File docs/clang_tool_refactoring.md (right):

https://codereview.chromium.org/2246263002/diff/60001/docs/clang_tool_refacto...
docs/clang_tool_refactoring.md:119: ```
Let's leave this out, it's a convenient shortcut but a bit fragile (similar to
how GYP + ninja was fragile). Hopefully we'll have something better in the
future.

https://codereview.chromium.org/2246263002/diff/60001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:351: // in one of
method declarations.
Let's file an upstream bug for this with the test case and reference it here: we
end up matching a ParmVarDecl for an unnamed thingie, but somehow it has a name.

https://codereview.chromium.org/2246263002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:356: if
(!loc.isMacroID() && old_text != original_name) {
Add a comment why we want to ignore SourceLocations that are in macros.

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

Daniel, can you take another look?

https://codereview.chromium.org/2246263002/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:534: 
On 2016/08/25 04:08:51, dcheng wrote:
> On 2016/08/15 23:05:08, Łukasz Anforowicz wrote:
> > 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'
> > 
> > ...
> 
> 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.

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.

https://codereview.chromium.org/2246263002/diff/60001/docs/clang_tool_refacto...
File docs/clang_tool_refactoring.md (right):

https://codereview.chromium.org/2246263002/diff/60001/docs/clang_tool_refacto...
docs/clang_tool_refactoring.md:119: ```
On 2016/08/25 04:08:51, dcheng wrote:
> Let's leave this out, it's a convenient shortcut but a bit fragile (similar to
> how GYP + ninja was fragile). Hopefully we'll have something better in the
> future.

Done.

https://codereview.chromium.org/2246263002/diff/60001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:351: // in one of
method declarations.
On 2016/08/25 04:08:51, dcheng wrote:
> Let's file an upstream bug for this with the test case and reference it here:
we
> end up matching a ParmVarDecl for an unnamed thingie, but somehow it has a
name.

Done: https://llvm.org/bugs/show_bug.cgi?id=29145

https://codereview.chromium.org/2246263002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:356: if
(!loc.isMacroID() && old_text != original_name) {
On 2016/08/25 04:08:52, dcheng wrote:
> Add a comment why we want to ignore SourceLocations that are in macros.

I've started using spelling location in the latest patchset - this lets me also
remove isMacroID call.  So - the comment above is no longer applicable.

[dcheng, 2016-08-26 20:28:02]

LGTM with a question

https://codereview.chromium.org/2246263002/diff/100001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:350: //
https://llvm.org/bugs/show_bug.cgi?id=29145
Perhaps describe the actual bug a bit here: a method of a templated class is
declared with a param with no name, and the definition has a name, and we
somehow end up trying to replace the parameter in the declaration as well.

Btw, did you try experimenting with checks to determine the SourceLocation is
"in range"? I tried that to and I recall it didn't work... but maybe its' worth
a shot now that we have a test case.

[Łukasz Anforowicz, 2016-08-26 22:39:51]

https://codereview.chromium.org/2246263002/diff/100001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2246263002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:350: //
https://llvm.org/bugs/show_bug.cgi?id=29145
On 2016/08/26 20:28:02, dcheng wrote:
> Perhaps describe the actual bug a bit here: a method of a templated class is
> declared with a param with no name, and the definition has a name, and we
> somehow end up trying to replace the parameter in the declaration as well.
 
Done.

> Btw, did you try experimenting with checks to determine the SourceLocation is
> "in range"? I tried that to and I recall it didn't work... but maybe its'
worth
> a shot now that we have a test case.

Good idea.  I cooked up something that only looks at the AST (checks if source
range of ParmVarDecl's parent contains ParmVarDecl - it doesn't in case of the
bug).

Wanna take one final look?

[Łukasz Anforowicz, 2016-08-26 22:40:00]

The CQ bit was checked by lukasza@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-26 22:40:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-08-26 22:58:12]

Still LGTM, thanks!

I don't really know if this is faster or not (I suspect not, calculating parents
is not free, but in theory, we only do it once per TU) and it looks a bit nicer
than comparing bits of text.

[Łukasz Anforowicz, 2016-08-27 00:17:20]

The CQ bit was unchecked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-08-27 00:17:26]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2016-08-27 00:18:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-27 00:25:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-27 00:27:26]

Description was changed from

==========
Skip ParmVarDecl nodes without a name.

BUG=598141
==========

to

==========
Skip ParmVarDecl nodes without a name.

BUG=598141

Committed: https://crrev.com/1333f1b925e384fe418c6ea9205b0383bd73f396
Cr-Commit-Position: refs/heads/master@{#414877}
==========

[commit-bot: I haz the power, 2016-08-27 00:27:27]

Patchset 7 (id:??) landed as
https://crrev.com/1333f1b925e384fe418c6ea9205b0383bd73f396
Cr-Commit-Position: refs/heads/master@{#414877}