Prepending "Get" when type with conflicting name is hidden inside return type.

Prepending "Get" when type with conflicting name is hidden inside return type.

BUG=582312
Committed: https://crrev.com/14d095b4df6754fa4e6959220b2b332db0b4f504
Cr-Commit-Position: refs/heads/master@{#417471}

[Łukasz Anforowicz, 2016-09-07 23:14:28]

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

[Łukasz Anforowicz, 2016-09-07 23:14:28]

Dana, can you please take a look?

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

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:305:
anyOf(hasDeclaration(namedDecl(hasName(name))), hasString(name)));
Without |hasString| we wouldn't properly handle the new test case from
functions-xxx.cc that goes through a typedef/using declaration:

class Foo582312 {};
using Bar = Foo582312;
static Bar* bar() {
  return nullptr;
}

I am not happy with |hasString| but I have tried various other things and I was
not able to work around the fact that QualType's -> goes directly to the final
type (so hasDeclaration matches the final type and not the intermediate types).

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:308:
hasDescendant(type_with_same_name_as_function)));
|hasDescendant| helps catch the RefPtr<SameNameAsFunction> case.

[danakj, 2016-09-08 23:30:40]

LGTM

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

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:305:
anyOf(hasDeclaration(namedDecl(hasName(name))), hasString(name)));
On 2016/09/07 23:14:28, Łukasz Anforowicz wrote:
> Without |hasString| we wouldn't properly handle the new test case from
> functions-xxx.cc that goes through a typedef/using declaration:
> 
> class Foo582312 {};
> using Bar = Foo582312;
> static Bar* bar() {
>   return nullptr;
> }
> 
> I am not happy with |hasString| but I have tried various other things and I
was
> not able to work around the fact that QualType's -> goes directly to the final
> type (so hasDeclaration matches the final type and not the intermediate
types).
> 

Can you leave a comment here saying that hasName() gives the true name, which
can differ from how it's written (ie typedefs) and hasString() gives the true
way it is written?

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/functions-original.cc (right):

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/functions-original.cc:17: }
nit: whitespace above and // namespace

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/methods-original.cc (right):

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:195: MyRefPtr<Baz>
baz() { return MyRefPtr<Baz>(); }
does it work for "const MyRefPtr<Baz>&" also? add testcase?

[Łukasz Anforowicz, 2016-09-09 00:20:35]

Thanks for reviewing.

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

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:305:
anyOf(hasDeclaration(namedDecl(hasName(name))), hasString(name)));
On 2016/09/08 23:30:40, danakj wrote:
> On 2016/09/07 23:14:28, Łukasz Anforowicz wrote:
> > Without |hasString| we wouldn't properly handle the new test case from
> > functions-xxx.cc that goes through a typedef/using declaration:
> > 
> > class Foo582312 {};
> > using Bar = Foo582312;
> > static Bar* bar() {
> >   return nullptr;
> > }
> > 
> > I am not happy with |hasString| but I have tried various other things and I
> was
> > not able to work around the fact that QualType's -> goes directly to the
final
> > type (so hasDeclaration matches the final type and not the intermediate
> types).
> > 
> 
> Can you leave a comment here saying that hasName() gives the true name, which
> can differ from how it's written (ie typedefs) and hasString() gives the true
> way it is written?

Done.

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/functions-original.cc (right):

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/functions-original.cc:17: }
On 2016/09/08 23:30:40, danakj wrote:
> nit: whitespace above and // namespace

Done.

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
File tools/clang/rewrite_to_chrome_style/tests/methods-original.cc (right):

https://codereview.chromium.org/2324643002/diff/1/tools/clang/rewrite_to_chro...
tools/clang/rewrite_to_chrome_style/tests/methods-original.cc:195: MyRefPtr<Baz>
baz() { return MyRefPtr<Baz>(); }
On 2016/09/08 23:30:40, danakj wrote:
> does it work for "const MyRefPtr<Baz>&" also? add testcase?

It works.  I've added a test.

[Łukasz Anforowicz, 2016-09-09 00:20:40]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-09-09 00:20:40]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2324643002/#ps20001
(title: "Addressed CR feedback from danakj@.")

[commit-bot: I haz the power, 2016-09-09 00:20:59]

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-09-09 01:25:42]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-09 01:27:11]

Description was changed from

==========
Prepending "Get" when type with conflicting name is hidden inside return type.

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

to

==========
Prepending "Get" when type with conflicting name is hidden inside return type.

BUG=582312

Committed: https://crrev.com/14d095b4df6754fa4e6959220b2b332db0b4f504
Cr-Commit-Position: refs/heads/master@{#417471}
==========

[commit-bot: I haz the power, 2016-09-09 01:27:11]

Patchset 2 (id:??) landed as
https://crrev.com/14d095b4df6754fa4e6959220b2b332db0b4f504
Cr-Commit-Position: refs/heads/master@{#417471}