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

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

BUG=676488
Committed: https://crrev.com/7d56282cc3b00bb07ebf0261f7185368b2456dce
Cr-Commit-Position: refs/heads/master@{#440778}

[Łukasz Anforowicz, 2016-12-21 23:48:39]

Description was changed from

==========
Supporting rewritign |to##macroArg()| into |To##macroArg()|.

BUG=676488
==========

to

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

BUG=676488
==========

[Łukasz Anforowicz, 2016-12-21 23:52:11]

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

[Łukasz Anforowicz, 2016-12-21 23:52:12]

Daniel, can you PTAL?

Nasko - not sure if I should land it, or if I should wait until somebody (you
can volunteer :-)) runs the modified tool and verifies that we haven't
regressed.

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

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

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:708: if
(!verified_out_of_scratch_space)
It turns out I implemented something similar, but in a slightly different way.

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:712: // Minimize
the diff - this is important for dealing with toFooBar ->
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)

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:724: spell,
spell.getLocWithOffset(expected_old_text.size()));
It's probably worth comparing the entire original argument to be safe, right?

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/macros-expected.cc (right):

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/macros-expected.cc:5: // Identifiers
in macros should never be rewritten, as the risk of things
Let's update this comment =)

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

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

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:708: if
(!verified_out_of_scratch_space)
On 2016/12/22 11:35:04, dcheng wrote:
> It turns out I implemented something similar, but in a slightly different way.

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.

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:712: // Minimize
the diff - this is important for dealing with toFooBar ->
On 2016/12/22 11:35:04, dcheng wrote:
> 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.

Ok - done.

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

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.

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:724: spell,
spell.getLocWithOffset(expected_old_text.size()));
On 2016/12/22 11:35:04, dcheng wrote:
> It's probably worth comparing the entire original argument to be safe, right?

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?

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/macros-expected.cc (right):

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/macros-expected.cc:5: // Identifiers
in macros should never be rewritten, as the risk of things
On 2016/12/22 11:35:04, dcheng wrote:
> Let's update this comment =)

Ooops :-)  Done (I've just removed the comment - it wasn't accurate to begin
with and after this CL will be even less useful).

[dcheng, 2016-12-22 21:32:42]

LGTM

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

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:724: spell,
spell.getLocWithOffset(expected_old_text.size()));
On 2016/12/22 17:59:30, Łukasz Anforowicz wrote:
> On 2016/12/22 11:35:04, dcheng wrote:
> > It's probably worth comparing the entire original argument to be safe,
right?
> 
> 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?

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

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

https://codereview.chromium.org/2592273002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:704:
result.SourceManager->getBufferName(spell) != "<scratch space>";
I think I would prefer that we just use the file entry, rather than comparing
against this internal string constant.

(It would be nice to share this helper with the edit tracker code, which I think
we can do just by having the utility helper return a SourceLocation, and having
it not really deal with filenames at all. But we can poke at this in a followup)

[Łukasz Anforowicz, 2016-12-23 00:17:49]

Daniel, you might want to take another look please?  I've had to make some big
changes in the last 2 patchsets.  I've talked with nasko@ and in theory the
bitset-fields-macro case we could fix up manually, but IMO it is more correct
(and much less risky) to avoid rewriting if the decl is non-rewritable.  The new
testcases should help understand why I am making the changes / why I am
reintroducing the decl_loc check.

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

https://codereview.chromium.org/2592273002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:724: spell,
spell.getLocWithOffset(expected_old_text.size()));
On 2016/12/22 21:32:41, dcheng wrote:
> On 2016/12/22 17:59:30, Łukasz Anforowicz wrote:
> > On 2016/12/22 11:35:04, dcheng wrote:
> > > It's probably worth comparing the entire original argument to be safe,
> right?
> > 
> > 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?
> 
> I don't think doing this hurts, so we may as well keep it for now? I'd be OK
> either way though.

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).

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

https://codereview.chromium.org/2592273002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:704:
result.SourceManager->getBufferName(spell) != "<scratch space>";
On 2016/12/22 21:32:42, dcheng wrote:
> I think I would prefer that we just use the file entry, rather than comparing
> against this internal string constant.
> 
> (It would be nice to share this helper with the edit tracker code, which I
think
> we can do just by having the utility helper return a SourceLocation, and
having
> it not really deal with filenames at all. But we can poke at this in a
followup)

I can't make it work without actually looking at "<scratch space>" string :-(  
I give up?

[dcheng, 2016-12-23 00:35:26]

The approach seems reasonable to me. Just one question

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

https://codereview.chromium.org/2592273002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:788: nullptr))
It seems a bit odd to call this twice; can we avoid that somehow?

[Łukasz Anforowicz, 2016-12-27 19:17:23]

Thanks.

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

https://codereview.chromium.org/2592273002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:788: nullptr))
On 2016/12/23 00:35:26, dcheng wrote:
> It seems a bit odd to call this twice; can we avoid that somehow?

First observation is that the call here is for |decl_loc| and the other call is
for |loc| (maybe I should rename |loc| to |target_loc|...).  So this is not
really the same call in all the cases (only when decl_loc == target_loc).

Second observation is that the question only deals with 1) performance and
(maybe) 2) maintainability / code duplication (although I don't currently see
how to improve in this area).  I think I'll just land this CL as-is, because I
think we are in agreement that the behavior is correct.

OTOH, I appreciate that you've pointed this out and I think that it makes sense
to consider memoization/caching to avoid calculating new_name for each
*reference* to a decl.  I'll try to run some perf tests later today to see if
memoization helps and how much.  Right now I think about having a *global* (not
per DeclRewriter) mapping from clang::NamedDecl* (stable, non-freed/reused
pointer I think) into new name (with special value of |old_name| indicating that
no renaming should be done - this might avoid repeated failed calls to
GenerateReplacement).

[Łukasz Anforowicz, 2016-12-27 19:47:05]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-12-27 19:47:06]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2592273002/#ps80001
(title: "Don't minimize the edit if not editing a macro.")

[commit-bot: I haz the power, 2016-12-27 19:47:15]

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-12-27 20:14:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482868025864560,
"parent_rev": "b166910ebc13944b28f090ac75ae454d78d45b3a", "commit_rev":
"d3efccaacb34c32ddb14dbd0f2dff90948e15bd1"}

[commit-bot: I haz the power, 2016-12-27 20:15:01]

Description was changed from

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

BUG=676488
==========

to

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

BUG=676488

Review-Url: https://codereview.chromium.org/2592273002
==========

[commit-bot: I haz the power, 2016-12-27 20:15:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-02 15:46:53]

Description was changed from

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

BUG=676488

Review-Url: https://codereview.chromium.org/2592273002
==========

to

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

BUG=676488

Committed: https://crrev.com/7d56282cc3b00bb07ebf0261f7185368b2456dce
Cr-Commit-Position: refs/heads/master@{#440778}
==========

[commit-bot: I haz the power, 2017-01-02 15:46:54]

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