PRESUBMIT.py: Check #include file order.

PRESUBMIT.py: Check #include file order.


BUG=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=167654

[marja, 2012-11-08 18:28:13]

Hi maruel,

I seem to be unable to get the #include order right in my CLs. Can we haz a
PRESUBMIT.py check for it?

[M-A Ruel, 2012-11-08 18:46:20]

It seems to be quite duplicating cpplint. I'd like Elliot to take a look.

https://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py#newcode591
PRESUBMIT.py:591: 
2 lines

[marja, 2012-11-08 19:05:24]

On 2012/11/08 18:46:20, Marc-Antoine Ruel wrote:
> It seems to be quite duplicating cpplint. I'd like Elliot to take a look.

What are the plans wrt cpplint? (Adding it to presubmit?) I just tried out
quickly, and it seems to catch lot of non-errors (e.g., macros) and a bunch of
real erros... So the code is not very cpplintable atm. :/

[Elliot Glaysher, 2012-11-08 19:23:55]

On 2012/11/08 19:05:24, marja wrote:
> On 2012/11/08 18:46:20, Marc-Antoine Ruel wrote:
> > It seems to be quite duplicating cpplint. I'd like Elliot to take a look.
> 
> What are the plans wrt cpplint? (Adding it to presubmit?) I just tried out
> quickly, and it seems to catch lot of non-errors (e.g., macros) and a bunch of
> real erros... So the code is not very cpplintable atm. :/

I don't really have an opinion here. I would note that the include order in
cpplint.py is marked alpha and I've seen it give false positives before.

[marja, 2012-11-08 19:26:36]

Yeah, cpplint chokes at least when there are #ifdefs, whereas this CL should
process them properly..

I'd suggest let's add this, even if it's duplicate, and remove it when there are
concrete plans how to proceed with cpplint.

[M-A Ruel, 2012-11-13 01:55:59]

https://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py#newcode520
PRESUBMIT.py:520: cpp_system_include_seen = False
You should use a one-variable state machine, it'd be simpler IMHO but I don't
mind.

https://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py#newcode562
PRESUBMIT.py:562: contents = input_api.ReadFile(f).splitlines()
That's slow. There are other functions that cache the file content, you should
use these.

[marja, 2012-11-13 17:30:19]

Thanks for comments!

http://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py#newcode520
PRESUBMIT.py:520: cpp_system_include_seen = False
On 2012/11/13 01:55:59, Marc-Antoine Ruel wrote:
> You should use a one-variable state machine, it'd be simpler IMHO but I don't
> mind.

Done. This was ugly.

http://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py#newcode562
PRESUBMIT.py:562: contents = input_api.ReadFile(f).splitlines()
On 2012/11/13 01:55:59, Marc-Antoine Ruel wrote:
> That's slow. There are other functions that cache the file content, you should
> use these.

Done (f.NewContents()).

http://codereview.chromium.org/11364156/diff/2001/PRESUBMIT.py#newcode591
PRESUBMIT.py:591: 
On 2012/11/08 18:46:20, Marc-Antoine Ruel wrote:
> 2 lines

Done.

[M-A Ruel, 2012-11-13 18:05:14]

https://codereview.chromium.org/11364156/diff/9001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11364156/diff/9001/PRESUBMIT.py#newcode520
PRESUBMIT.py:520: class State:
C_SYSTEM_INCLUDES, CPP_SYSTEM_INCLUDES, CUSTOM_INCLUDES = range(3)

and you made a typo. Please test your code.

[marja, 2012-11-13 18:12:47]

Oops! :/ Yes, now tested also with data that makes it go into that branch.

http://codereview.chromium.org/11364156/diff/9001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/11364156/diff/9001/PRESUBMIT.py#newcode520
PRESUBMIT.py:520: class State:
On 2012/11/13 18:05:14, Marc-Antoine Ruel wrote:
> C_SYSTEM_INCLUDES, CPP_SYSTEM_INCLUDES, CUSTOM_INCLUDES = range(3)
> 
> and you made a typo. Please test your code.

Done.

[M-A Ruel, 2012-11-13 18:18:01]

lgtm

https://codereview.chromium.org/11364156/diff/14001/PRESUBMIT.py
File PRESUBMIT.py (right):

https://codereview.chromium.org/11364156/diff/14001/PRESUBMIT.py#newcode520
PRESUBMIT.py:520: class State:
No need for class State at all. It'll make the code less verbose and still
readable.

https://codereview.chromium.org/11364156/diff/14001/PRESUBMIT.py#newcode573
PRESUBMIT.py:573: wanted_line = ('#include "%s"' % f.LocalPath().replace('.cc',
'.h'))
expected = '#include "%s"' % f.LocalPath().replace('.cc', '.h')

[marja, 2012-11-13 18:24:56]

Thanks for review!

http://codereview.chromium.org/11364156/diff/14001/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/11364156/diff/14001/PRESUBMIT.py#newcode520
PRESUBMIT.py:520: class State:
On 2012/11/13 18:18:01, Marc-Antoine Ruel wrote:
> No need for class State at all. It'll make the code less verbose and still
> readable.

Done.

http://codereview.chromium.org/11364156/diff/14001/PRESUBMIT.py#newcode573
PRESUBMIT.py:573: wanted_line = ('#include "%s"' % f.LocalPath().replace('.cc',
'.h'))
On 2012/11/13 18:18:01, Marc-Antoine Ruel wrote:
> expected = '#include "%s"' % f.LocalPath().replace('.cc', '.h')

Done.

[marja, 2012-11-13 18:32:16]

(And I'll commit this tomorrow morning (UTC+1) when I'm at work again; in case
it breaks something...)

[tfarina, 2012-11-13 19:22:42]

http://codereview.chromium.org/11364156/diff/11002/PRESUBMIT.py
File PRESUBMIT.py (right):

http://codereview.chromium.org/11364156/diff/11002/PRESUBMIT.py#newcode639
PRESUBMIT.py:639: results.extend(_CheckNoTrinaryTrueFalse(input_api,
output_api))
unrelated: s/Trinary/Ternary, no?