898274 - Check header ordering in |make check-style|

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 880088 implements |make check-style|, but the patch in that bug doesn't do the #include statement order checking.  This bug can be about that, now that bug 888088 is done and they should be in right order.

Since the order checking will need to understand #ifdefs to some degree, I might implement #ifndef wrapper checking at the same time, i.e.:

- #ifndef wrappers should have the right form. (XXX: not yet implemented)
  - Every header file should have one.
  - It should be in the vanilla form and have no tokens before/after it so
    that GCC and clang can avoid multiple-inclusion.
  - The guard name used should be appropriate for the filename.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

You should be able to reuse part of the script I wrote for this, especially if it needs to understand #ifdefs. I.. don't know if the code is in a state where that will be easy, but give it a look.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Fix ordering of various #include statements.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Check ordering of #include statements in check_spidermonkey_style.py.

Note that config/check_spidermonkey_style.py and
js/src/config/check_spidermonkey_style.py are (and must be) identical.

The trickiest part about all this is the handling of #if/#endif blocks, which
I've described in the comments.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

FYI:

config/check_spidermonkey_style.py:7:80: E501 line too long (91 > 79 characters)
config/check_spidermonkey_style.py:13:2: W291 trailing whitespace
config/check_spidermonkey_style.py:19:2: W291 trailing whitespace
config/check_spidermonkey_style.py:92:80: E501 line too long (80 > 79 characters)
config/check_spidermonkey_style.py:94:80: E501 line too long (88 > 79 characters)
config/check_spidermonkey_style.py:95:80: E501 line too long (80 > 79 characters)
config/check_spidermonkey_style.py:111:80: E501 line too long (82 > 79 characters)
config/check_spidermonkey_style.py:178:16: E711 comparison to None should be 'if cond is not None:'
config/check_spidermonkey_style.py:222:80: E501 line too long (81 > 79 characters)
config/check_spidermonkey_style.py:223:80: E501 line too long (87 > 79 characters)
config/check_spidermonkey_style.py:301:80: E501 line too long (100 > 79 characters)
config/check_spidermonkey_style.py:313:20: E703 statement ends with a semicolon
config/check_spidermonkey_style.py:327:12: W291 trailing whitespace
config/check_spidermonkey_style.py:335:80: E501 line too long (101 > 79 characters)
config/check_spidermonkey_style.py:338:80: E501 line too long (85 > 79 characters)
config/check_spidermonkey_style.py:369:1: W293 blank line contains whitespace
config/check_spidermonkey_style.py:376:21: E703 statement ends with a semicolon
config/check_spidermonkey_style.py:379:80: E501 line too long (83 > 79 characters)
config/check_spidermonkey_style.py:418:5: E303 too many blank lines (2)
config/check_spidermonkey_style.py:422:80: E501 line too long (95 > 79 characters)
config/check_spidermonkey_style.py:436:80: E501 line too long (86 > 79 characters)
config/check_spidermonkey_style.py:437:80: E501 line too long (87 > 79 characters)
config/check_spidermonkey_style.py:441:80: E501 line too long (84 > 79 characters)
config/check_spidermonkey_style.py:447:80: E501 line too long (93 > 79 characters)
config/check_spidermonkey_style.py:449:80: E501 line too long (91 > 79 characters)
config/check_spidermonkey_style.py:452:80: E501 line too long (80 > 79 characters)
config/check_spidermonkey_style.py:455:5: E303 too many blank lines (2)
config/check_spidermonkey_style.py:467:80: E501 line too long (96 > 79 characters)
config/check_spidermonkey_style.py:468:80: E501 line too long (80 > 79 characters)
config/check_spidermonkey_style.py:469:80: E501 line too long (85 > 79 characters)
config/check_spidermonkey_style.py:472:80: E501 line too long (94 > 79 characters)
config/check_spidermonkey_style.py:474:5: E303 too many blank lines (2)
config/check_spidermonkey_style.py:475:27: E127 continuation line over-indented for visual indent
config/check_spidermonkey_style.py:477:80: E501 line too long (85 > 79 characters)
config/check_spidermonkey_style.py:493:1: W293 blank line contains whitespace
config/check_spidermonkey_style.py:570:80: E501 line too long (130 > 79 characters)

Comment 5

[Nicholas Nethercote [inactive]]

FWIW, PEP 8 was recently updated (http://hg.python.org/peps/rev/fb24c80e9afb) and it now says "Aim to limit all lines to a maximum of 79 characters, but up to 99 characters is acceptable when it improves readability."  Given that SpiderMonkey has a 99-char line length for C++ code, I've chosen to interpret that liberally :)

I'm happy to fix the other things before landing.

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Check ordering of #include statements in check_spidermonkey_style.py.

This version fixes the non-line-length style nits.

Comment 7

[:Benjamin Peterson]

Comment on attachment 789860 [details] [diff] [review]
(part 2) - Check ordering of #include statements in check_spidermonkey_style.py.

Review of attachment 789860 [details] [diff] [review]:
-----------------------------------------------------------------

Besides some trivial stuff, I'm mostly curious about the need for the tree structure representation of include blocks.

::: config/check_spidermonkey_style.py
@@ +88,5 @@
>      'unicode/utypes.h',         # ICU
>      'vtune/VTuneWrapper.h'      # VTune
>  ])
>  
> +# We don't check the #include ordering of these files, because they are strange.

This comment make it sounds like these files themselves aren't checked. In fact, it means includes of them aren't checked in other files. (I hope that makes sense.)

@@ +297,5 @@
>  
> +def module_name(name):
> +    '''Strip the trailing .cpp, .h, inlines.h or -inl.h from a filename.'''
> +
> +    return name.replace('inlines.h', '').replace('-inl.h', '').replace('.h', '').replace('.cpp', '')

Interesting solution!

@@ +300,5 @@
> +
> +    return name.replace('inlines.h', '').replace('-inl.h', '').replace('.h', '').replace('.cpp', '')
> +
> +
> +class IInclude(object):

What does the first I in the name stand for? Maybe t this could be called "Include" and the next class "IncludeBlock"?

@@ +308,5 @@
> +        self.inclname = inclname
> +        self.linenum = linenum
> +        self.is_system = is_system
> +
> +    def isLeaf(self):

It's weird that only this function uses camelCase.

@@ +369,5 @@
> +
> +    Each leaf is either an IInclude (representing a #include), or another
> +    nested IBlock.'''
> +    def __init__(self):
> +        self.leaves = []

Technically these aren't leaves, since there can be nested blocks.

@@ +416,5 @@
> +
> +    def check_include_statement(include):
> +        '''Check the style of a single #include statement.'''
> +
> +        if (include.is_system):

parens not needed.

@@ +417,5 @@
> +    def check_include_statement(include):
> +        '''Check the style of a single #include statement.'''
> +
> +        if (include.is_system):
> +            # Check it is not a known local file (in which case it's probably a system header).

I think an "if" is missing from this comment. Also, aren't we checking if it _is_ a known local file?

@@ +425,5 @@
> +                      include.quote() + ' should be included using',
> +                      'the #include "..." form')
> +
> +        else:
> +            if include.inclname not in included_inclnames_to_ignore:

Presumably this could be lifted to the above if statement to become |elif include.inclname not in included_inclnames_to_ignore| but maybe you have stylistic reasons for the current form?

@@ +468,5 @@
> +
> +    # The #include statements in the files in assembler/ and yarr/ have all manner of implicit
> +    # ordering requirements.  Boo.  Ignore them.
> +    skip_order_checking = inclname.startswith('assembler/') or \
> +                          inclname.startswith('yarr/')

startswith takes a tuple, so this can be expressed as inclname.startswith(("assembler/", "yarr/")).

@@ +481,5 @@
> +        else:
> +            for prev2, this2 in zip([None] + this.leaves[0:-1], this.leaves):
> +                pair_traverse(prev2, this2)
> +
> +    pair_traverse(None, block_stack[-1])

Why do you even need this tree structure of include blocks? Wouldn't it be simpler to parse the includes as a linear list of blocks? If it was just a linear list of blocks, you could walk that with a normal for loop rather than this recursive function.

Maybe I'm missing something?

Comment 8

[Nicholas Nethercote [inactive]]

> Why do you even need this tree structure of include blocks?

I tried to explain that with this comment:

+#   Note that the presence of #if/#endif blocks complicates things, to the
+#   point that it's not always clear where a conditionally-compiled #include
+#   statement should go, even to a human.  Therefore, we check the #include
+#   statements within each #if/#endif block in isolation, but don't try to do
+#   any order checking between such blocks.

But clearly it wasn't enough.  Maybe I should add an example?  I could also move the comment down to be just above the IInclude/IBlock classes?

Comment 9

[:Benjamin Peterson]

Right, I understand the need to consider each chunk of includes separately, but why do you need a tree for that?

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to :Benjamin Peterson from comment #9)
> Right, I understand the need to consider each chunk of includes separately,
> but why do you need a tree for that?

Oh right.  In case of nested #ifdef blocks.

Comment 11

[:Benjamin Peterson]

But you don't use the tree structure to do validate anything right? You could just as well flatten it to a list of include blocks?

Comment 12

[Nicholas Nethercote [inactive]]

I do use the tree structure.  Any #if/#endif block, regardless of nesting depth, is checked in isolation.

Comment 13

[:Benjamin Peterson]

Comment on attachment 789860 [details] [diff] [review]
(part 2) - Check ordering of #include statements in check_spidermonkey_style.py.

Review of attachment 789860 [details] [diff] [review]:
-----------------------------------------------------------------

Alright then modulo the other things.

Comment 14

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/200fae26b271
https://hg.mozilla.org/integration/mozilla-inbound/rev/64e83ea4fbad

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/200fae26b271
https://hg.mozilla.org/mozilla-central/rev/64e83ea4fbad