Blacklisting renaming of "hash" in case of static methods and functions.

Blacklisting renaming of "hash" in case of static methods and functions.

BUG=677166
Committed: https://crrev.com/1b4b746ad2d2c288bdc568a046b2461b637a75fd
Cr-Commit-Position: refs/heads/master@{#440878}

[Łukasz Anforowicz, 2016-12-27 19:08:44]

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

[Łukasz Anforowicz, 2016-12-27 19:08:44]

Daniel, can you PTAL?

This CL adds blacklisting of "hash" when it is the name of 1) a static method or
2) a name of a function.

This CL doesn't change renaming of "begin" and "swap", but adds more tests for
these cases.

[dcheng, 2016-12-27 20:30:40]

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

https://codereview.chromium.org/2608443002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:210: bool
IsBlacklistedFunctionOrInstanceOrStaticMethodName(llvm::StringRef name) {
I find the naming of this helper to be a bit confusing. In clang parlance, I
would expect this to just be called IsBlacklistedFunctionName().

https://codereview.chromium.org/2608443002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:218: return name ==
"swap";
And call this "IsBlacklistedFreeFunctionName" (since in clang parlance, methods
are a subclass of functions, while this explicitly excludes checking against
method names)

[Łukasz Anforowicz, 2016-12-27 22:42:43]

Daniel, I've addressed the feedback + also moved the code around a little bit. 
Can you take another look please?

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

https://codereview.chromium.org/2608443002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:210: bool
IsBlacklistedFunctionOrInstanceOrStaticMethodName(llvm::StringRef name) {
On 2016/12/27 20:30:40, dcheng wrote:
> I find the naming of this helper to be a bit confusing. In clang parlance, I
> would expect this to just be called IsBlacklistedFunctionName().

Done.

https://codereview.chromium.org/2608443002/diff/20001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:218: return name ==
"swap";
On 2016/12/27 20:30:40, dcheng wrote:
> And call this "IsBlacklistedFreeFunctionName" (since in clang parlance,
methods
> are a subclass of functions, while this explicitly excludes checking against
> method names)

Done.

[dcheng, 2016-12-28 18:48:49]

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

https://codereview.chromium.org/2608443002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:247: return true;
I think we can just early return here if it's static, right?

https://codereview.chromium.org/2608443002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:868: static bool
IsBlacklistedMethodName(llvm::StringRef name) {
How come this is a static method now?

[Łukasz Anforowicz, 2016-12-28 18:56:50]

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

https://codereview.chromium.org/2608443002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:247: return true;
On 2016/12/28 18:48:49, dcheng wrote:
> I think we can just early return here if it's static, right?

Done.

https://codereview.chromium.org/2608443002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:868: static bool
IsBlacklistedMethodName(llvm::StringRef name) {
On 2016/12/28 18:48:49, dcheng wrote:
> How come this is a static method now?

This was a free function, but the only caller was/is
GuessNameForUnresolvedDependentNode below, so I think it is cleaner if
IsBlacklistedMethodName is a static, private method.

Maybe it would help if I explained that IsBlacklistedMethodName covers names of
both instance and static methods?  Such combination is not covered by any of the
other blacklisting helpers above.

[dcheng, 2016-12-28 19:03:38]

LGTM with comment

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

https://codereview.chromium.org/2608443002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:868: static bool
IsBlacklistedMethodName(llvm::StringRef name) {
On 2016/12/28 18:56:50, Łukasz Anforowicz wrote:
> On 2016/12/28 18:48:49, dcheng wrote:
> > How come this is a static method now?
> 
> This was a free function, but the only caller was/is
> GuessNameForUnresolvedDependentNode below, so I think it is cleaner if
> IsBlacklistedMethodName is a static, private method.
> 
> Maybe it would help if I explained that IsBlacklistedMethodName covers names
of
> both instance and static methods?  Such combination is not covered by any of
the
> other blacklisting helpers above.

I think it's just confusing to not have all the similar helpers grouped
together. I think it's OK that it's only really used by one thing.

[Łukasz Anforowicz, 2016-12-28 19:08:07]

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

https://codereview.chromium.org/2608443002/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:868: static bool
IsBlacklistedMethodName(llvm::StringRef name) {
On 2016/12/28 19:03:38, dcheng wrote:
> On 2016/12/28 18:56:50, Łukasz Anforowicz wrote:
> > On 2016/12/28 18:48:49, dcheng wrote:
> > > How come this is a static method now?
> > 
> > This was a free function, but the only caller was/is
> > GuessNameForUnresolvedDependentNode below, so I think it is cleaner if
> > IsBlacklistedMethodName is a static, private method.
> > 
> > Maybe it would help if I explained that IsBlacklistedMethodName covers names
> of
> > both instance and static methods?  Such combination is not covered by any of
> the
> > other blacklisting helpers above.
> 
> I think it's just confusing to not have all the similar helpers grouped
> together. I think it's OK that it's only really used by one thing.

Done.

[Łukasz Anforowicz, 2016-12-28 19:08:12]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2016-12-28 19:08:12]

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/2608443002/#ps80001
(title: "Making IsBlacklistedMethodName a free function again.")

[commit-bot: I haz the power, 2016-12-28 19:08:26]

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-28 19:16:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482952092189300,
"parent_rev": "424e832f4542c977df6c5138e9bda2362ca8098d", "commit_rev":
"205125fb998f6ef68de60365e971082f85fb2d48"}

[commit-bot: I haz the power, 2016-12-28 19:17:19]

Description was changed from

==========
Blacklisting renaming of "hash" in case of static methods and functions.

BUG=677166
==========

to

==========
Blacklisting renaming of "hash" in case of static methods and functions.

BUG=677166

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

[commit-bot: I haz the power, 2016-12-28 19:17:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2017-01-02 15:49:51]

Description was changed from

==========
Blacklisting renaming of "hash" in case of static methods and functions.

BUG=677166

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

to

==========
Blacklisting renaming of "hash" in case of static methods and functions.

BUG=677166

Committed: https://crrev.com/1b4b746ad2d2c288bdc568a046b2461b637a75fd
Cr-Commit-Position: refs/heads/master@{#440878}
==========

[commit-bot: I haz the power, 2017-01-02 15:49:52]

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