Closed Bug 898274 Opened 6 years ago Closed 6 years ago

Check header ordering in |make check-style|

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: njn, Assigned: njn)

References

Details

(Whiteboard: [js:t])

Attachments

(2 files, 1 obsolete file)

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.
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.
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.
Attachment #789413 - Flags: review?(benjamin)
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)
Attachment #789412 - Flags: review?(benjamin) → review+
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.
This version fixes the non-line-length style nits.
Attachment #789860 - Flags: review?(benjamin)
Attachment #789413 - Attachment is obsolete: true
Attachment #789413 - Flags: review?(benjamin)
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?
> 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?
Right, I understand the need to consider each chunk of includes separately, but why do you need a tree for that?
(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.
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?
I do use the tree structure.  Any #if/#endif block, regardless of nesting depth, is checked in isolation.
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.
Attachment #789860 - Flags: review?(benjamin) → review+
https://hg.mozilla.org/mozilla-central/rev/200fae26b271
https://hg.mozilla.org/mozilla-central/rev/64e83ea4fbad
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 932991
You need to log in before you can comment on or make changes to this bug.