Prepend "Get" to method name if method name conflicts with return type name.

Prepend "Get" to method name if method name conflicts with return type name.

BUG=582312
Committed: https://crrev.com/93f68599689997ab8d48961c221ee4a3564a5d60
Cr-Commit-Position: refs/heads/master@{#414550}

[Łukasz Anforowicz, 2016-08-24 18:09:49]

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

[Łukasz Anforowicz, 2016-08-24 18:09:50]

Daniel, can you take a look please?  This CL implements esprehn@'s suggestion
from https://crbug.com/582312#c17.  I've tried running the tool on wtf and it
seems to 1) work for BitVector.h and OutOfLineBits and 2) not break anything
else (i.e. unresolved/dependent renames are obviously unaware of conflicts and
so will never prepend "Get").

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:340:
clang::ASTContext& context,
|match| function call below takes a non-const ASTContext& as an argument.

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:357:
clang::ASTContext& context,
Best to be consistent here (i.e. use non-const in all overloads) - otherwise
overload resolution + templates is very confusing (at one point I had non-const
only in 2 places - it resulted in a warning that one of the GetNameForDecl
overloads is unused... :-/ ).

[dcheng, 2016-08-25 17:36:00]

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:347:
hasCanonicalType(hasDeclaration(namedDecl(hasName(name))));
Can we just pull the return type out of the FunctionDecl rather than running a
matcher?

I think we can just do:
  decl.getReturnType()->getBaseTypeIdentifier()->getName()

Which has the advantage that it'll work with multiple layers of indirection.

[Łukasz Anforowicz, 2016-08-25 18:30:40]

Daniel, can you take another look please?

Also: while I think this CL moves things in the right direction (and is
landable), I've just realized that looking at the method's return type might be
insufficient - i.e. see https://crbugs.com/582312#c57 (where ColorFilter is an
enum, but method colorFilter returns SkColorFilter*).

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:347:
hasCanonicalType(hasDeclaration(namedDecl(hasName(name))));
On 2016/08/25 17:36:00, dcheng wrote:
> Can we just pull the return type out of the FunctionDecl rather than running a
> matcher?
> 
> I think we can just do:
>   decl.getReturnType()->getBaseTypeIdentifier()->getName()
> 
> Which has the advantage that it'll work with multiple layers of indirection.

Thanks - done.  I didn't know about getBaseTypeIdentifier - I was using the
matcher mainly to strip pointers+references, but your suggestion is obviously
better.

RE: multiple layers of indirection - this might also be a problem in
https://codereview.chromium.org/2256913002 - I've left a comment there.

[dcheng, 2016-08-25 21:00:57]

On 2016/08/25 18:30:40, Łukasz Anforowicz wrote:
> Daniel, can you take another look please?
> 
> Also: while I think this CL moves things in the right direction (and is
> landable), I've just realized that looking at the method's return type might
be
> insufficient - i.e. see https://crbugs.com/582312#c57 (where ColorFilter is an
> enum, but method colorFilter returns SkColorFilter*).

Yeah, that's OK: we don't need the tool to handle every possible combination of
things, just most of everything.

> 
>
https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
> File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):
> 
>
https://codereview.chromium.org/2276983002/diff/1/tools/clang/rewrite_to_chro...
> tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:347:
> hasCanonicalType(hasDeclaration(namedDecl(hasName(name))));
> On 2016/08/25 17:36:00, dcheng wrote:
> > Can we just pull the return type out of the FunctionDecl rather than running
a
> > matcher?
> > 
> > I think we can just do:
> >   decl.getReturnType()->getBaseTypeIdentifier()->getName()
> > 
> > Which has the advantage that it'll work with multiple layers of indirection.
> 
> Thanks - done.  I didn't know about getBaseTypeIdentifier - I was using the
> matcher mainly to strip pointers+references, but your suggestion is obviously
> better.
> 
> RE: multiple layers of indirection - this might also be a problem in
> https://codereview.chromium.org/2256913002 - I've left a comment there.

LGTM, hopefully it's less of a problem for templates as well. I'm cautiously
optimistic that maybe we can just ignore it there and do a few manual fixups.

[Łukasz Anforowicz, 2016-08-25 21:15:46]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-08-25 21:15:46]

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/2276983002/#ps60001
(title: "Rebasing on ToT (more or less).")

[commit-bot: I haz the power, 2016-08-25 21:16:43]

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-25 22:00:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-25 22:02:55]

Description was changed from

==========
Prepend "Get" to method name if method name conflicts with return type name.

BUG=582312
==========

to

==========
Prepend "Get" to method name if method name conflicts with return type name.

BUG=582312

Committed: https://crrev.com/93f68599689997ab8d48961c221ee4a3564a5d60
Cr-Commit-Position: refs/heads/master@{#414550}
==========

[commit-bot: I haz the power, 2016-08-25 22:02:56]

Patchset 4 (id:??) landed as
https://crrev.com/93f68599689997ab8d48961c221ee4a3564a5d60
Cr-Commit-Position: refs/heads/master@{#414550}