ui/app_list: Enable a set of cpplint checks.

ui/app_list: Enable a set of cpplint checks.

That were otherwise filtered out (disabled) by the way depot_tools'
presubmit_canned_checks.py::CheckChangeLintsClean() worked.

As of
https://src.chromium.org/viewvc/chrome?revision=293357&view=revision, it
was changed, so clients can pass their own files and enable those that
were disabled.

BUG=None
TEST=None
R=tapted@chromium.org

Committed: https://crrev.com/f3e59936e42bcee3383dbdde3f35f92db0509dc8
Cr-Commit-Position: refs/heads/master@{#308481}

[tfarina, 2014-12-14 03:35:59]

ptal

https://codereview.chromium.org/804763002/diff/1/ui/app_list/PRESUBMIT.py
File ui/app_list/PRESUBMIT.py (right):

https://codereview.chromium.org/804763002/diff/1/ui/app_list/PRESUBMIT.py#new...
ui/app_list/PRESUBMIT.py:26: input_api, output_api, sources, '')
Is this correct?

Hopefully, this will help us to catch everything (that were always regressing)
again.

[tapted, 2014-12-15 22:12:55]

https://codereview.chromium.org/804763002/diff/1/ui/app_list/PRESUBMIT.py
File ui/app_list/PRESUBMIT.py (right):

https://codereview.chromium.org/804763002/diff/1/ui/app_list/PRESUBMIT.py#new...
ui/app_list/PRESUBMIT.py:26: input_api, output_api, sources, '')
On 2014/12/14 03:35:59, tfarina wrote:
> Is this correct?

nah - it should be []

i.e.

  return input_api.canned_checks.CheckChangeLintsClean(
      input_api, output_api, sources, [])


lgtm after that

[tfarina, 2014-12-15 23:45:19]

Pushing to CQ.

https://codereview.chromium.org/804763002/diff/1/ui/app_list/PRESUBMIT.py
File ui/app_list/PRESUBMIT.py (right):

https://codereview.chromium.org/804763002/diff/1/ui/app_list/PRESUBMIT.py#new...
ui/app_list/PRESUBMIT.py:26: input_api, output_api, sources, '')
On 2014/12/15 22:12:55, tapted wrote:
> On 2014/12/14 03:35:59, tfarina wrote:
> > Is this correct?
> 
> nah - it should be []
> 
> i.e.
> 
>   return input_api.canned_checks.CheckChangeLintsClean(
>       input_api, output_api, sources, [])
> 
> 
> lgtm after that
Thanks for being  patience with me in these reviews. You see I'm weak on Python.
Weird language, the lack of type is so annoying and forces you to guess types
(so for me, that is not familiar with the language, causes this), much like
JavaScript. 

Done.

[tfarina, 2014-12-15 23:45:26]

The CQ bit was checked by tfarina@chromium.org

[Nico, 2014-12-15 23:46:41]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-12-15 23:46:41]

How long do the new presubmit checks take to run?

[commit-bot: I haz the power, 2014-12-15 23:46:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/804763002/20001

[commit-bot: I haz the power, 2014-12-16 00:55:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-16 00:56:26]

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

[tapted, 2014-12-16 02:27:05]

On 2014/12/15 23:46:41, Nico wrote:
> How long do the new presubmit checks take to run?

For this particular change, it just alters how error messages are filtered.
cpplint.py seems to do the work detect things regardless of the filters, so this
particular CL shouldn't change things.

cpplint is currently run for any changed file under src/chrome (with some
filters). Currently src/ui/app_list is the only thing under src/ui that has
cpplint run during presubmit (now, with less error messages filtered out).
Again, just changed files. But as it stands, changing a file under
src/ui/app_list shouldn't tickle any extra work compared to changing a file
under src/chrome

I suspect it would be "nice" for more things to be running cpplint (e.g. under
src/ui and elsewhere beyond just src/chrome). If clang-format catches on (it has
already in ui/app_list), there's probably scope to absorb things we want out of
cpplint.py into it and scrap the separate python check.

I haven't done any performance checking on cpplint.py itself (thiago?).

[Nico, 2014-12-17 00:12:44]

On Mon, Dec 15, 2014 at 6:27 PM, <tapted@chromium.org> wrote:
>
> On 2014/12/15 23:46:41, Nico wrote:
>
>> How long do the new presubmit checks take to run?
>>
>
> For this particular change, it just alters how error messages are filtered.
> cpplint.py seems to do the work detect things regardless of the filters,
> so this
> particular CL shouldn't change things.
>
> cpplint is currently run for any changed file under src/chrome (with some
> filters). Currently src/ui/app_list is the only thing under src/ui that has
> cpplint run during presubmit (now, with less error messages filtered out).
> Again, just changed files. But as it stands, changing a file under
> src/ui/app_list shouldn't tickle any extra work compared to changing a file
> under src/chrome
>
> I suspect it would be "nice" for more things to be running cpplint (e.g.
> under
> src/ui and elsewhere beyond just src/chrome).


Out of interest, which of cpplint's checks are useful for chromium?


> If clang-format catches on (it has
> already in ui/app_list), there's probably scope to absorb things we want
> out of
> cpplint.py into it and scrap the separate python check.
>
> I haven't done any performance checking on cpplint.py itself (thiago?).
>
> https://codereview.chromium.org/804763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2014-12-17 00:41:52]

On 2014/12/17 00:12:44, Nico wrote:
> On Mon, Dec 15, 2014 at 6:27 PM, <mailto:tapted@chromium.org> wrote:
> > I suspect it would be "nice" for more things to be running cpplint (e.g.
> > under
> > src/ui and elsewhere beyond just src/chrome).
> 
> 
> Out of interest, which of cpplint's checks are useful for chromium?

I'd be inclined to say "most of them" :). There's a long list -
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/cppl...
. I don't know details about all of them, and won't try to classify them. I
think many are things that an experienced programmer would never do anyway, just
out of instinct.

I'm indifferent about `you forgot to #include <algorithm>`. However, a few
things I occasionally see that are useful: readability/braces for picking up
broken `if/else` attachment (e.g. http://crrev.com/367843004 ), `Use of C-style
cast` (although it's broken in Objective C), `Single-argument constructor should
be explicit`.

[Nico, 2014-12-17 00:50:40]

On Tue, Dec 16, 2014 at 4:41 PM, <tapted@chromium.org> wrote:
>
> On 2014/12/17 00:12:44, Nico wrote:
>
>> On Mon, Dec 15, 2014 at 6:27 PM, <mailto:tapted@chromium.org> wrote:
>> > I suspect it would be "nice" for more things to be running cpplint (e.g.
>> > under
>> > src/ui and elsewhere beyond just src/chrome).
>>
>
>
>  Out of interest, which of cpplint's checks are useful for chromium?
>>
>
> I'd be inclined to say "most of them" :). There's a long list -
> https://code.google.com/p/chromium/codesearch#chromium/
> tools/depot_tools/cpplint.py&q=cpplint.py&sq=package:chromium&l=177
> . I don't know details about all of them, and won't try to classify them. I
> think many are things that an experienced programmer would never do
> anyway, just
> out of instinct.
>

Are these documented somewhere? (Didn't find anything in 10s of googling)

Looking over the list and judging by the name, many of these look like
things that the compiler should (and does) catch – it can do this much
faster, and more reliably.


> I'm indifferent about `you forgot to #include <algorithm>`. However, a few
> things I occasionally see that are useful: readability/braces for picking
> up
> broken `if/else` attachment (e.g. http://crrev.com/367843004 ),


That's cool, but I think that's one thing the compiler should (but doesn't
yet) do: http://llvm.org/bugs/show_bug.cgi?id=18938


> `Use of C-style
> cast` (although it's broken in Objective C), `Single-argument constructor
> should
> be explicit`.
>
> https://codereview.chromium.org/804763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tapted, 2014-12-17 02:02:24]

On 2014/12/17 00:50:40, Nico wrote:
> On Tue, Dec 16, 2014 at 4:41 PM, <mailto:tapted@chromium.org> wrote:
> >
> > On 2014/12/17 00:12:44, Nico wrote:
> >
> >> On Mon, Dec 15, 2014 at 6:27 PM, <mailto:tapted@chromium.org> wrote:
> >> > I suspect it would be "nice" for more things to be running cpplint (e.g.
> >> > under
> >> > src/ui and elsewhere beyond just src/chrome).
> >>
> >
> >
> >  Out of interest, which of cpplint's checks are useful for chromium?
> >>
> >
> > I'd be inclined to say "most of them" :). There's a long list -
> > https://code.google.com/p/chromium/codesearch#chromium/
> > tools/depot_tools/cpplint.py&q=cpplint.py&sq=package:chromium&l=177
> > . I don't know details about all of them, and won't try to classify them. I
> > think many are things that an experienced programmer would never do
> > anyway, just
> > out of instinct.
> >
> 
> Are these documented somewhere? (Didn't find anything in 10s of googling)

I couldn't find much either. But some horrible regex like `sed -ne "/[^)]$/ {H};
/[)]$/ {H;x; s/\n//g; s/  //g; p}" cpplint.py  | grep -o 'error([^)]*)' | sed -r
's/[^,]*,[^,]*,[^,]* (.*)/\1/'| sort` can suck out some more descriptive
strings: e.g.

'readability/alt_tokens', 2,'Use operator %s instead of %s' %
(_ALT_TOKEN_REPLACEMENT[match.group(1)
'readability/braces', 4,'Did you mean "else if"? If not, start a new line for
"if".')
'readability/braces', 4,'Else clause should be indented at the same level as if.
''Ambiguous nested if/else chains require braces.')
'readability/braces', 4,'If/else bodies with multiple statements require
braces')
'readability/braces', 4,'If/else bodies with multiple statements require
braces')
'readability/braces', 4,"You don't need a ; after a }")

> Looking over the list and judging by the name, many of these look like
> things that the compiler should (and does) catch – it can do this much
> faster, and more reliably.

yep - agree :). It looks like go/clangtidy wants to replace cpplint eventually.

[Nico, 2014-12-17 02:06:11]

On Tue, Dec 16, 2014 at 6:02 PM, <tapted@chromium.org> wrote:
>
> On 2014/12/17 00:50:40, Nico wrote:
>
>> On Tue, Dec 16, 2014 at 4:41 PM, <mailto:tapted@chromium.org> wrote:
>> >
>> > On 2014/12/17 00:12:44, Nico wrote:
>> >
>> >> On Mon, Dec 15, 2014 at 6:27 PM, <mailto:tapted@chromium.org> wrote:
>> >> > I suspect it would be "nice" for more things to be running cpplint
>> (e.g.
>> >> > under
>> >> > src/ui and elsewhere beyond just src/chrome).
>> >>
>> >
>> >
>> >  Out of interest, which of cpplint's checks are useful for chromium?
>> >>
>> >
>> > I'd be inclined to say "most of them" :). There's a long list -
>> > https://code.google.com/p/chromium/codesearch#chromium/
>> > tools/depot_tools/cpplint.py&q=cpplint.py&sq=package:chromium&l=177
>> > . I don't know details about all of them, and won't try to classify
>> them. I
>> > think many are things that an experienced programmer would never do
>> > anyway, just
>> > out of instinct.
>> >
>>
>
>  Are these documented somewhere? (Didn't find anything in 10s of googling)
>>
>
> I couldn't find much either. But some horrible regex like `sed -ne
> "/[^)]$/ {H};
> /[)]$/ {H;x; s/\n//g; s/  //g; p}" cpplint.py  | grep -o 'error([^)]*)' |
> sed -r
> 's/[^,]*,[^,]*,[^,]* (.*)/\1/'| sort` can suck out some more descriptive
> strings: e.g.
>
> 'readability/alt_tokens', 2,'Use operator %s instead of %s' %
> (_ALT_TOKEN_REPLACEMENT[match.group(1)
> 'readability/braces', 4,'Did you mean "else if"? If not, start a new line
> for
> "if".')
> 'readability/braces', 4,'Else clause should be indented at the same level
> as if.
> ''Ambiguous nested if/else chains require braces.')
> 'readability/braces', 4,'If/else bodies with multiple statements require
> braces')
> 'readability/braces', 4,'If/else bodies with multiple statements require
> braces')
> 'readability/braces', 4,"You don't need a ; after a }")
>
>  Looking over the list and judging by the name, many of these look like
>> things that the compiler should (and does) catch – it can do this much
>> faster, and more reliably.
>>
>
> yep - agree :). It looks like go/clangtidy
> <https://goto.google.com/clangtidy> wants to replace cpplint eventually.
>

clangtidy is also a separate step, and since it builds ASTs and whatnot, it
needs defines and includes paths – I'd guess it'll be slower than cpplint.
For some things, clang-tidy will make sense, but "important" things should
just be caught by the "regular" compiler.


>
> https://codereview.chromium.org/804763002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.