Blacklisting renaming of |size()| and |length()| methods.

Blacklisting renaming of |size()| and |length()| methods.

BUG=672902
Review-Url: https://codereview.chromium.org/2657773003
Cr-Commit-Position: refs/heads/master@{#446667}
Committed: https://chromium.googlesource.com/chromium/src/+/265ae0467148c0872ccae6c862813d27f21ce031

[Łukasz Anforowicz, 2017-01-26 00:19:58]

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

[Łukasz Anforowicz, 2017-01-26 00:19:58]

Daniel or Dana - could you PTAL?

[Łukasz Anforowicz, 2017-01-26 00:59:26]

Description was changed from

==========
Blacklisting renaming of |size()| method.

BUG=672902
==========

to

==========
Blacklisting renaming of |size()| and |length()| methods.

BUG=672902
==========

[Łukasz Anforowicz, 2017-01-26 01:02:08]

I've also added blacklisting of |length| for good measure...

I assume that name-based blacklisting (without checking if length/size have zero
parameters) is okay - the tool is supposed to handle 99% of cases and the
remaining ones can be tackled manually.

I also assume that |size| and |length| are desirable even though they deviate a
bit from the Blink Style Guide, which theoretically would prefer to see |Size()|
or |GetLength()| (there is a potential conflict with Length type :-)

[Łukasz Anforowicz, 2017-01-26 01:06:36]

Description was changed from

==========
Blacklisting renaming of |size()| and |length()| methods.

BUG=672902
==========

to

==========
Blacklisting renaming of |size()| and |length()| methods.

BUG=672902
==========

[dcheng, 2017-01-26 01:57:52]

On 2017/01/26 01:02:08, Łukasz Anforowicz wrote:
> I've also added blacklisting of |length| for good measure...
> 
> I assume that name-based blacklisting (without checking if length/size have
zero
> parameters) is okay - the tool is supposed to handle 99% of cases and the
> remaining ones can be tackled manually.
> 
> I also assume that |size| and |length| are desirable even though they deviate
a
> bit from the Blink Style Guide, which theoretically would prefer to see
|Size()|
> or |GetLength()| (there is a potential conflict with Length type :-)

Ideally, we'd only do it on WTF stuff. Is there a way to scope it there? I think
we'd still want to call the other ones Size() / GetLength() for consistency.

[Łukasz Anforowicz, 2017-01-26 17:18:48]

On 2017/01/26 01:57:52, dcheng wrote:
> On 2017/01/26 01:02:08, Łukasz Anforowicz wrote:
> > I've also added blacklisting of |length| for good measure...
> > 
> > I assume that name-based blacklisting (without checking if length/size have
> zero
> > parameters) is okay - the tool is supposed to handle 99% of cases and the
> > remaining ones can be tackled manually.
> > 
> > I also assume that |size| and |length| are desirable even though they
deviate
> a
> > bit from the Blink Style Guide, which theoretically would prefer to see
> |Size()|
> > or |GetLength()| (there is a potential conflict with Length type :-)
> 
> Ideally, we'd only do it on WTF stuff. Is there a way to scope it there? I
think
> we'd still want to call the other ones Size() / GetLength() for consistency.

I'd like to argue for not special-casing WTF namespace in this CL:

1. We don't distinguish between WTF and blink namespaces elsewhere in the clang
tool.  We used to do this for type traits, but we stopped because of
https://crbug.com/640749#c1.  |length| is similar in this regard - some |length|
methods are problematic inside blink namespace - for example some |length|
methods are blacklisted because of IDL scraping, but this breaks templates (with
unresolved template params) that refer to |length| (CSSGroupingRule::length is
IDL-unrelated method and LiveCSSRuleList::length for a template [and I can't
find the specific IDL-originating |length| for this case right now that I run
into yesterday]).

2. With unresolved template parameters, we only know the namespace where the
template parameter is declared, but not where the actual template argument will
come from.  For example, when looking at the template in
mojo/public/cpp/bindings/lib/serialization.h, we don't know if |input| in
|input.size()| refers to an object from WTF vs blink vs std namespace.

3. Consistently skipping |size| and |length| everywhere feels more robust -
adding heuristics here (i.e. WTF vs blink namespace, or unresolved-templates vs
resolved-decls) feels more likely to result in some gotchas and compilation
errors down the road.


I've also thought about looking at parameter count and it also feels difficult
to pull off.  In particular, in case of an unresolved, template-param-based
using decl (i.e using BaseTemplate::someMethod) the parameter count is not
known.  We could add some heuristics for this one case, and compare parameter
count for other cases (where it is known AFAIK), but I'd rather not do that
without a strong, compelling benefit (i.e. I think that manually tweaking
renaming after the Great Blink Rename is an okay option if we care about
PascalCasing some of |size()| and |length()| methods).

[dcheng, 2017-01-27 08:59:32]

I suppose fixing up non-WTF size() / length() naming after the rename itself.
LGTM.

[Łukasz Anforowicz, 2017-01-27 14:54:16]

The CQ bit was checked by lukasza@chromium.org

[commit-bot: I haz the power, 2017-01-27 14:54:32]

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, 2017-01-27 15:08:02]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1485528856600460,
"parent_rev": "ccc612ebe5cf17b20d71f5eced95cba4b04e4ca0", "commit_rev":
"265ae0467148c0872ccae6c862813d27f21ce031"}

[commit-bot: I haz the power, 2017-01-27 15:08:38]

Description was changed from

==========
Blacklisting renaming of |size()| and |length()| methods.

BUG=672902
==========

to

==========
Blacklisting renaming of |size()| and |length()| methods.

BUG=672902

Review-Url: https://codereview.chromium.org/2657773003
Cr-Commit-Position: refs/heads/master@{#446667}
Committed:
https://chromium.googlesource.com/chromium/src/+/265ae0467148c0872ccae6c86281...
==========

[commit-bot: I haz the power, 2017-01-27 15:08:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/265ae0467148c0872ccae6c86281...