Renaming blink method names inside gmock's EXPECT_CALL macro invocation.

Renaming blink method names inside gmock's EXPECT_CALL macro invocation.

BUG=677544
Review-Url: https://codereview.chromium.org/2609473002
Cr-Commit-Position: refs/heads/master@{#442347}
Committed: https://chromium.googlesource.com/chromium/src/+/b5ca969e8001ef2d6292e92f0d1203e02fba30ec

[Łukasz Anforowicz, 2016-12-29 21:14:02]

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

[Łukasz Anforowicz, 2016-12-29 21:14:02]

Daniel, can you PTAL?

I've tested it on Source/web/tests and it seems to have done a good job.

I am not very happy about just trying locations one-by-one and seeing which one
is the right one, but it works + will only be done for gmock (because of
mocksMethod matcher) + I don't know how to do it differently.

WDYT?

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

Description was changed from

==========
Renaming blink method names inside gmock's EXPECT_CALL macro invocation.

BUG=
==========

to

==========
Renaming blink method names inside gmock's EXPECT_CALL macro invocation.

BUG=677544
==========

[dcheng, 2017-01-06 06:58:46]

I'm wondering... can we use clang's preprocessor hooks (PPCallbacks) to track
macro expansions of EXPECT_CALL in conjunction with AST matchers to do this in a
more natural way? Scanning backwards for the macro arguments is a bit of a hack
=)

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

https://codereview.chromium.org/2609473002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:745:
clang::CharSourceRange* out_range) {
Stylistically, I'm not a fan of optional output arguments. When I see this, it
feels like a function is doing "too much". This is similar to my earlier
feedback about GenerateReplacement().

[Łukasz Anforowicz, 2017-01-07 01:16:30]

On 2017/01/06 06:58:46, dcheng wrote:
> I'm wondering... can we use clang's preprocessor hooks (PPCallbacks) to track
> macro expansions of EXPECT_CALL in conjunction with AST matchers to do this in
a
> more natural way? Scanning backwards for the macro arguments is a bit of a
hack
> =)

Thanks for the suggestion and for the help with figuring out how to hook-up
PPCallbacks to ClangTool (via SourceFileCallbacks).  Can you take another look
please?

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

https://codereview.chromium.org/2609473002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:745:
clang::CharSourceRange* out_range) {
On 2017/01/06 06:58:45, dcheng wrote:
> Stylistically, I'm not a fan of optional output arguments. When I see this, it
> feels like a function is doing "too much". This is similar to my earlier
> feedback about GenerateReplacement().

I've inlined GetActualText back into the only caller that remained after
addressing your other piece of feedback / after using PPCallbacks.

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

https://codereview.chromium.org/2609473002/diff/60001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1002: // know how
to get clang::SourceManager to call getFileName.
This inaccuracy is probably okay - even if there are other macros called
EXPECT_CALL that take 2 arguments, we should be fine because of additional
filtering done by the |mocksMethod| custom matcher.

https://codereview.chromium.org/2609473002/diff/60001/tools/clang/scripts/run...
File tools/clang/scripts/run_tool.py (right):

https://codereview.chromium.org/2609473002/diff/60001/tools/clang/scripts/run...
tools/clang/scripts/run_tool.py:172: sys.stderr.write(result['stderr_text'])
This is useful.  I am too lazy to do this in a separate CL? :-/

[dcheng, 2017-01-09 19:27:47]

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

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:112: if
(potentially_mocked_method->getNumParams() != Node.getNumParams())
It's too bad it's not easy to do something like use the list of overrides here
=)

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:975:
PPCallbacks(GMockMemberRewriter* rewriter) : rewriter_(rewriter) {}
Nit: explicit

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1151:
std::move(new_callbacks), std::move(pp_callbacks_));
Do we need chaining support? It seems like we our code should be simple enough
that we don't need chaining.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1159:
assert(source_counter_ == 1);  // We only have *one* pp_callbacks_.
What happens if the tool is run on multiple files at once?

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1162:
compiler.getPreprocessor().addPPCallbacks(std::move(pp_callbacks_));
Similarly, I think we should be able to assert pp_callbacks_ here?

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1551: // Run all
the matchers.
This comment seems a bit out-of-place, since it looks like it applies to the
part where we hook up the PPCallbacks.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/gmock-original.cc (right):

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/gmock-original.cc:11: virtual void
myMethod(int my_param){};
Nit: remove ;

[Łukasz Anforowicz, 2017-01-09 20:11:27]

Thanks for the feedback - can you take another look please?

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

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:112: if
(potentially_mocked_method->getNumParams() != Node.getNumParams())
On 2017/01/09 19:27:47, dcheng wrote:
> It's too bad it's not easy to do something like use the list of overrides here
> =)

Acknowledged.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:975:
PPCallbacks(GMockMemberRewriter* rewriter) : rewriter_(rewriter) {}
On 2017/01/09 19:27:47, dcheng wrote:
> Nit: explicit

Ooops.  Done.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1151:
std::move(new_callbacks), std::move(pp_callbacks_));
On 2017/01/09 19:27:47, dcheng wrote:
> Do we need chaining support? It seems like we our code should be simple enough
> that we don't need chaining.

Done.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1159:
assert(source_counter_ == 1);  // We only have *one* pp_callbacks_.
On 2017/01/09 19:27:47, dcheng wrote:
> What happens if the tool is run on multiple files at once?

Fixed by creating a new PPCallbacks every time.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1162:
compiler.getPreprocessor().addPPCallbacks(std::move(pp_callbacks_));
On 2017/01/09 19:27:47, dcheng wrote:
> Similarly, I think we should be able to assert pp_callbacks_ here?

See above - we are now creating a new PPCallbacks every time.

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1551: // Run all
the matchers.
On 2017/01/09 19:27:47, dcheng wrote:
> This comment seems a bit out-of-place, since it looks like it applies to the
> part where we hook up the PPCallbacks.

I changed the comment - does it look better now?

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/tests/gmock-original.cc (right):

https://codereview.chromium.org/2609473002/diff/80001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/tests/gmock-original.cc:11: virtual void
myMethod(int my_param){};
On 2017/01/09 19:27:47, dcheng wrote:
> Nit: remove ;

Done.

[dcheng, 2017-01-09 20:37:55]

lgtm

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

https://codereview.chromium.org/2609473002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1143:
SourceFileCallbacks(GMockMemberRewriter* gmock_member_rewriter)
explicit ;-)

https://codereview.chromium.org/2609473002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1541: // Prepare
and run the tool.
To me, this comment doesn't add much. *shrug*

[Łukasz Anforowicz, 2017-01-09 20:54:55]

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

https://codereview.chromium.org/2609473002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1143:
SourceFileCallbacks(GMockMemberRewriter* gmock_member_rewriter)
On 2017/01/09 20:37:54, dcheng wrote:
> explicit ;-)

Doh... :-/   Done.

https://codereview.chromium.org/2609473002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1541: // Prepare
and run the tool.
On 2017/01/09 20:37:54, dcheng wrote:
> To me, this comment doesn't add much. *shrug*

My motivation for adding a comment here is to have some kind of visual delimiter
between the matchers/rewriters above and where the tool is run.  When I was
debugging something I tried commenting out matchers via #if 0 and accidentally
commented out creation of the |factory| below - this is probably a user error,
but I think the comment would have helped me see that factory is not part of the
matcher setup above it.

[Łukasz Anforowicz, 2017-01-09 20:54:59]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-01-09 20:54:59]

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/2609473002/#ps120001
(title: "Make SourceFileCallbacks's constructor explicit.")

[commit-bot: I haz the power, 2017-01-09 20:55:48]

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

[dcheng, 2017-01-09 21:13:57]

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

https://codereview.chromium.org/2609473002/diff/100001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1541: // Prepare
and run the tool.
On 2017/01/09 20:54:55, Łukasz Anforowicz wrote:
> On 2017/01/09 20:37:54, dcheng wrote:
> > To me, this comment doesn't add much. *shrug*
> 
> My motivation for adding a comment here is to have some kind of visual
delimiter
> between the matchers/rewriters above and where the tool is run.  When I was
> debugging something I tried commenting out matchers via #if 0 and accidentally
> commented out creation of the |factory| below - this is probably a user error,
> but I think the comment would have helped me see that factory is not part of
the
> matcher setup above it.

Ah, I see. One thing we could do is hoist the matcher building and registration
into a Register() method on the MatchCallback implementations. This is useful
for things like unit testing... Which we don't have. And it also means we need
actual classes instead of just type aliases..

[commit-bot: I haz the power, 2017-01-09 21:18:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483995299102700,
"parent_rev": "0649171aca96b82408f54611cd76bea80eee3ad2", "commit_rev":
"b5ca969e8001ef2d6292e92f0d1203e02fba30ec"}

[commit-bot: I haz the power, 2017-01-09 21:18:51]

Description was changed from

==========
Renaming blink method names inside gmock's EXPECT_CALL macro invocation.

BUG=677544
==========

to

==========
Renaming blink method names inside gmock's EXPECT_CALL macro invocation.

BUG=677544

Review-Url: https://codereview.chromium.org/2609473002
Cr-Commit-Position: refs/heads/master@{#442347}
Committed:
https://chromium.googlesource.com/chromium/src/+/b5ca969e8001ef2d6292e92f0d12...
==========

[commit-bot: I haz the power, 2017-01-09 21:18:52]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/b5ca969e8001ef2d6292e92f0d12...