Add lint check against "Foo *bar" and "Foo &bar" declarations.

Add lint check against "Foo *bar" and "Foo &bar" declarations.

Depends on extension mechanism for cpplint.py: http://codereview.appspot.com/4950069/

Pulls r74 of cpplint.py from:
http://google-styleguide.googlecode.com/svn-history/r74/trunk/cpplint/cpplint.py

Taken from WebKit's fork of cpplint.py.

WebKit patch was: http://trac.webkit.org/changeset/46856

Credit Torch Mobile, Inc. who have contributed the WebKit patch in question.

BUG=none
TEST=Run gcl lint on a CL that has a Foo *bar style declaration.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=100151

[M-A Ruel, 2011-09-06 16:03:20]

Elliot manages this file.

[Elliot Glaysher, 2011-09-06 17:34:14]

On 2011/09/06 16:03:20, Marc-Antoine Ruel wrote:
> Elliot manages this file.

This isn't the canonical version of the file. That is here:
http://google-styleguide.googlecode.com/svn/trunk/cpplint/

Please submit a patch against that directory (with unit test) and I'll copy the
data over.

[Alexei Svitkine (slow), 2011-09-07 15:00:01]

Looks like the regular google style guide allows both "int *foo" and "int* foo",
so these checks are not appropriate there. The Chromium style guide only allows
"int* foo", of course.

I've updated the patch to propose a way to extend cpplint.py with
Chromium-specific checks, while still re-using its infrastructure.

Marc-Antoine / Elliot: What do you think of this approach?

(Obviously, the cpplint.py changes would need to be upstreamed).

[Elliot Glaysher, 2011-09-07 18:09:59]

Realistically, I have no problem just reverting the revert and
committing your original patch upstream. In practice, the majority of
opensource Google C++ projects are part of the chromium sphere anyway.

On Wed, Sep 7, 2011 at 8:00 AM,  <asvitkine@chromium.org> wrote:
> Looks like the regular google style guide allows both "int *foo" and "int*
> foo",
> so these checks are not appropriate there. The Chromium style guide only
> allows
> "int* foo", of course.
>
> I've updated the patch to propose a way to extend cpplint.py with
> Chromium-specific checks, while still re-using its infrastructure.
>
> Marc-Antoine / Elliot: What do you think of this approach?

If you're fine with this check not happening on the codereview tool,
this approach is fine. Not all developers use gcl either. If you're
going to do things this way, I guess this looks fine. But since this
touches gcl, I'll wait for maruel's input.

> (Obviously, the cpplint.py changes would need to be upstreamed).

Especially since I'm currently working on porting a year backlog of
patches against the internal cpplint.py right now; if this isn't
upstreamed, it will be clobbered in a matter of days.

> http://codereview.chromium.org/7834045/

[M-A Ruel, 2011-09-07 19:18:29]

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py
File cpplint_chromium.py (right):

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py#newcode1
cpplint_chromium.py:1: #!/usr/bin/python2.4
Ugh? Why this copyright? Where did you copy that from?

You should just use the standard chromium copyright here if that's new code.

Also, this file is not executable so no shebang.

http://codereview.chromium.org/7834045/diff/8001/gcl.py
File gcl.py (left):

http://codereview.chromium.org/7834045/diff/8001/gcl.py#oldcode1176
gcl.py:1176: 
keep 2 lines between file level symbols as per style guide.

[Alexei Svitkine (slow), 2011-09-07 19:46:06]

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py
File cpplint_chromium.py (right):

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py#newcode1
cpplint_chromium.py:1: #!/usr/bin/python2.4
On 2011/09/07 19:18:29, Marc-Antoine Ruel wrote:
> Ugh? Why this copyright? Where did you copy that from?
> 
> You should just use the standard chromium copyright here if that's new code.
> 
> Also, this file is not executable so no shebang.

Sorry, should have included the context:

This is a change that was contributed to WebKit's fork of our cpplint.py.

WebKit patch was: http://trac.webkit.org/changeset/46856

Credit Torch Mobile, Inc. who have contributed the WebKit patch in
question.

[M-A Ruel, 2011-09-07 19:52:07]

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py
File cpplint_chromium.py (right):

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py#newcode1
cpplint_chromium.py:1: #!/usr/bin/python2.4
On 2011/09/07 19:46:06, Alexei Svitkine wrote:
> On 2011/09/07 19:18:29, Marc-Antoine Ruel wrote:
> > Ugh? Why this copyright? Where did you copy that from?
> > 
> > You should just use the standard chromium copyright here if that's new code.
> > 
> > Also, this file is not executable so no shebang.
> 
> Sorry, should have included the context:
> 
> This is a change that was contributed to WebKit's fork of our cpplint.py.
> 
> WebKit patch was: http://trac.webkit.org/changeset/46856
> 
> Credit Torch Mobile, Inc. who have contributed the WebKit patch in
> question.

Ok, but remove the shebang.

[Alexei Svitkine (slow), 2011-09-07 20:00:37]

On 2011/09/07 18:09:59, Elliot Glaysher wrote:
> Realistically, I have no problem just reverting the revert and
> committing your original patch upstream. In practice, the majority of
> opensource Google C++ projects are part of the chromium sphere anyway.

I am told that a lot of google3 uses the other style. Does google3 use our
cpplint.py in any way?

Also asan, tsan, etc. tools apparently use the other style too.

Elliot, maybe it make more sense to add an option to cpplint.py to enable this
check? Then, Chromium could set that option to true when running cpplint.py.

What do you think?

[Elliot Glaysher, 2011-09-07 20:09:28]

On Wed, Sep 7, 2011 at 1:00 PM,  <asvitkine@chromium.org> wrote:
> On 2011/09/07 18:09:59, Elliot Glaysher wrote:
>>
>> Realistically, I have no problem just reverting the revert and
>> committing your original patch upstream. In practice, the majority of
>> opensource Google C++ projects are part of the chromium sphere anyway.
>
> I am told that a lot of google3 uses the other style. Does google3 use our
> cpplint.py in any way?

No. They use the copy in p4, from which this was sanitized/checks that
don't make sense removed/proprietary info/paths/names redacted. (In
the last round of porting yesterday after you reminded me of cpplint's
existence, I took 6 patches and rejected 5 for open sourcing.) Changes
flow from inside to outside; most of the patches from external
contributor wouldn't interest people inside google3 land and aren't
ported to the version in p4.

> Also asan, tsan, etc. tools apparently use the other style too.

That makes me a little sad.

> Elliot, maybe it make more sense to add an option to cpplint.py to enable
> this
> check? Then, Chromium could set that option to true when running cpplint.py.
>
> What do you think?

Adding the option to gcl would be preferable to this. If it's behind
an option, I fear no one will ever run it in the other contexts.

-- Elliot

> http://codereview.chromium.org/7834045/

[Alexei Svitkine (slow), 2011-09-07 20:27:57]

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py
File cpplint_chromium.py (right):

http://codereview.chromium.org/7834045/diff/8001/cpplint_chromium.py#newcode1
cpplint_chromium.py:1: #!/usr/bin/python2.4
On 2011/09/07 19:52:07, Marc-Antoine Ruel wrote:
> On 2011/09/07 19:46:06, Alexei Svitkine wrote:
> > On 2011/09/07 19:18:29, Marc-Antoine Ruel wrote:
> > > Ugh? Why this copyright? Where did you copy that from?
> > > 
> > > You should just use the standard chromium copyright here if that's new
code.
> > > 
> > > Also, this file is not executable so no shebang.
> > 
> > Sorry, should have included the context:
> > 
> > This is a change that was contributed to WebKit's fork of our cpplint.py.
> > 
> > WebKit patch was: http://trac.webkit.org/changeset/46856
> > 
> > Credit Torch Mobile, Inc. who have contributed the WebKit patch in
> > question.
> 
> Ok, but remove the shebang.

Done.

http://codereview.chromium.org/7834045/diff/8001/gcl.py
File gcl.py (left):

http://codereview.chromium.org/7834045/diff/8001/gcl.py#oldcode1176
gcl.py:1176: 
On 2011/09/07 19:18:29, Marc-Antoine Ruel wrote:
> keep 2 lines between file level symbols as per style guide.

Done.

[M-A Ruel, 2011-09-07 20:30:40]

lgtm but I don't think "cpplint_chromium.py" will pass the presubmit check,
you'll have to add it to the blacklist in PRESUBMIT.py since it doesn't have the
right copyright and isn't in third_party.

Elliot, I think it'd be better to move cpplint.py to third_party so it'd be
clear that it's a third party. :)

We can keep a shim in depot_tools/ if needed.

[Elliot Glaysher, 2011-09-07 20:38:49]

On 2011/09/07 20:30:40, Marc-Antoine Ruel wrote:
> Elliot, I think it'd be better to move cpplint.py to third_party so it'd be
> clear that it's a third party. :)

The last time I looked at doing this, depot_tools couldn't DEPS depend on other
repositories. Has that changed?

[M-A Ruel, 2011-09-07 20:42:55]

On 2011/09/07 20:38:49, Elliot Glaysher wrote:
> On 2011/09/07 20:30:40, Marc-Antoine Ruel wrote:
> > Elliot, I think it'd be better to move cpplint.py to third_party so it'd be
> > clear that it's a third party. :)
> 
> The last time I looked at doing this, depot_tools couldn't DEPS depend on
other
> repositories. Has that changed?

I mean svn mv cpplint.py third_party.
Just so it's clear it comes from elsewhere.

[Elliot Glaysher, 2011-09-07 20:45:38]

On 2011/09/07 20:42:55, Marc-Antoine Ruel wrote:
> On 2011/09/07 20:38:49, Elliot Glaysher wrote:
> > On 2011/09/07 20:30:40, Marc-Antoine Ruel wrote:
> > > Elliot, I think it'd be better to move cpplint.py to third_party so it'd
be
> > > clear that it's a third party. :)
> > 
> > The last time I looked at doing this, depot_tools couldn't DEPS depend on
> other
> > repositories. Has that changed?
> 
> I mean svn mv cpplint.py third_party.
> Just so it's clear it comes from elsewhere.

Oh, in that case sure. Alexei, feel free to do that to silence the presubmit
warning.

[Alexei Svitkine (slow), 2011-09-07 20:48:05]

> Oh, in that case sure. Alexei, feel free to do that to silence the presubmit
> warning.

Done.

I've also spun off the cpplint.py changes to upstream:
http://codereview.appspot.com/4950069/

[M-A Ruel, 2011-09-07 20:48:10]

Le 7 septembre 2011 16:45, <erg@chromium.org> a écrit :

> Oh, in that case sure. Alexei, feel free to do that to silence the
> presubmit
> warning.


I recommend doing this in a separate change though.

[Alexei Svitkine (slow), 2011-09-07 20:52:05]

On 2011/09/07 20:48:10, Marc-Antoine Ruel wrote:
> Le 7 septembre 2011 16:45,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=erg@chromium.org> a écrit :
> 
> > Oh, in that case sure. Alexei, feel free to do that to silence the
> > presubmit
> > warning.
> 
> 
> I recommend doing this in a separate change though.

Done: http://codereview.chromium.org/7789033/

[M-A Ruel, 2011-09-07 21:44:26]

cpplint.py is not in this change anymore?

[Alexei Svitkine (slow), 2011-09-08 13:13:55]

On 2011/09/07 21:44:26, Marc-Antoine Ruel wrote:
> cpplint.py is not in this change anymore?

I've re-added it - pulled from upstream r74.

[M-A Ruel, 2011-09-08 13:14:37]

lgtm

[commit-bot: I haz the power, 2011-09-08 13:15:58]

Can't process patch for file cpplint_chromium.py.
Failed to parse svn properties.

[commit-bot: I haz the power, 2011-09-08 15:27:06]

Can't process patch for file cpplint_chromium.py.
Failed to parse svn properties.

[commit-bot: I haz the power, 2011-09-08 15:34:48]

Change committed as 100151