Bug 898274 (part 2) - Check ordering of #include statements in check_spidermonkey_style.py. r=benjamin.
authorNicholas Nethercote <nnethercote@mozilla.com>
Wed, 14 Aug 2013 21:59:57 -0700
changeset 142713 64e83ea4fbadf65a1152b0fd2344631314ceb0c5
parent 142712 200fae26b2717a4c5a8aa9a399ee7a675cb9be1b
child 142714 31c08ca022b3e811cf371142d86d3229ada27d59
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersbenjamin
bugs898274
milestone26.0a1
Bug 898274 (part 2) - Check ordering of #include statements in check_spidermonkey_style.py. r=benjamin.
config/check_spidermonkey_style.py
js/src/config/check_spidermonkey_style.py
js/src/tests/style/BadIncludes.h
js/src/tests/style/BadIncludesOrder-inl.h
js/src/tests/style/HeaderCycleB4-inl.h
--- a/config/check_spidermonkey_style.py
+++ b/config/check_spidermonkey_style.py
@@ -5,37 +5,39 @@
 
 #----------------------------------------------------------------------------
 # This script checks various aspects of SpiderMonkey code style.  The current checks are as
 # follows.
 #
 # We check the following things in headers.
 #
 # - No cyclic dependencies.
-# 
+#
 # - No normal header should #include a inlines.h/-inl.h file.
-# 
+#
 # - #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.
-# 
+#
 # We check the following things in all files.
 #
 # - #includes should have full paths, e.g. "jit/Ion.h", not "Ion.h".
 #
 # - #includes should use the appropriate form for system headers (<...>) and
 #   local headers ("...").
 #
-# - #includes should be ordered correctly. (XXX: not yet implemented;  see bug
-#   888088)
+# - #includes should be ordered correctly.
 #   - Each one should be in the correct section.
 #   - Alphabetical order should be used within sections.
 #   - Sections should be in the right order.
+#   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 (including nested ones) in
+#   isolation, but don't try to do any order checking between such blocks.
 #----------------------------------------------------------------------------
 
 from __future__ import print_function
 
 import difflib
 import os
 import re
 import subprocess
@@ -82,43 +84,74 @@ included_inclnames_to_ignore = set([
     'unicode/udatpg.h',         # ICU
     'unicode/uenum.h',          # ICU
     'unicode/unum.h',           # ICU
     'unicode/ustring.h',        # ICU
     'unicode/utypes.h',         # ICU
     'vtune/VTuneWrapper.h'      # VTune
 ])
 
+# These files have additional constraints on where they are #included, so we
+# ignore #includes of them when checking #include ordering.
+oddly_ordered_inclnames = set([
+    'ctypes/typedefs.h',        # Included multiple times in the body of ctypes/CTypes.h
+    'jsautokw.h',               # Included in the body of frontend/TokenStream.h
+    'jswin.h',                  # Must be #included before <psapi.h>
+    'winbase.h',                # Must precede other system headers(?)
+    'windef.h'                  # Must precede other system headers(?)
+])
+
 # The files in tests/style/ contain code that fails this checking in various
 # ways.  Here is the output we expect.  If the actual output differs from
 # this, one of the following must have happened.
 # - New SpiderMonkey code violates one of the checked rules.
 # - The tests/style/ files have changed without expected_output being changed
 #   accordingly.
 # - This script has been broken somehow.
 #
 expected_output = '''\
 js/src/tests/style/BadIncludes2.h:1: error:
     vanilla header includes an inline-header file "tests/style/BadIncludes2-inl.h"
 
-js/src/tests/style/BadIncludes.h:1: error:
+js/src/tests/style/BadIncludes.h:3: error:
     the file includes itself
 
-js/src/tests/style/BadIncludes.h:3: error:
+js/src/tests/style/BadIncludes.h:6: error:
     "BadIncludes2.h" is included using the wrong path;
     did you forget a prefix, or is the file not yet committed?
 
-js/src/tests/style/BadIncludes.h:4: error:
+js/src/tests/style/BadIncludes.h:8: error:
     <tests/style/BadIncludes2.h> should be included using
     the #include "..." form
 
-js/src/tests/style/BadIncludes.h:5: error:
+js/src/tests/style/BadIncludes.h:10: error:
     "stdio.h" is included using the wrong path;
     did you forget a prefix, or is the file not yet committed?
 
+js/src/tests/style/BadIncludesOrder-inl.h:5:6: error:
+    "vm/Interpreter-inl.h" should be included after "jsscriptinlines.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:6:7: error:
+    "jsscriptinlines.h" should be included after "js/Value.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:7:8: error:
+    "js/Value.h" should be included after "ds/LifoAlloc.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:8:9: error:
+    "ds/LifoAlloc.h" should be included after "jsapi.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:9:10: error:
+    "jsapi.h" should be included after <stdio.h>
+
+js/src/tests/style/BadIncludesOrder-inl.h:10:11: error:
+    <stdio.h> should be included after "mozilla/HashFunctions.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:27:28: error:
+    "jsobj.h" should be included after "jsfun.h"
+
 (multiple files): error:
     header files form one or more cycles
 
    tests/style/HeaderCycleA1.h
    -> tests/style/HeaderCycleA2.h
       -> tests/style/HeaderCycleA3.h
          -> tests/style/HeaderCycleA1.h
 
@@ -138,18 +171,18 @@ actual_output = []
 
 def out(*lines):
     for line in lines:
         actual_output.append(line + '\n')
 
 
 def error(filename, linenum, *lines):
     location = filename
-    if linenum != None:
-        location += ":" + str(linenum)
+    if linenum is not None:
+        location += ':' + str(linenum)
     out(location + ': error:')
     for line in (lines):
         out('    ' + line)
     out('')
 
 
 class FileKind(object):
     C = 1
@@ -158,37 +191,37 @@ class FileKind(object):
     H = 4
     TBL = 5
     MSG = 6
 
     @staticmethod
     def get(filename):
         if filename.endswith('.c'):
             return FileKind.C
-       
+
         if filename.endswith('.cpp'):
             return FileKind.CPP
-       
+
         if filename.endswith(('inlines.h', '-inl.h')):
             return FileKind.INL_H
 
         if filename.endswith('.h'):
             return FileKind.H
 
         if filename.endswith('.tbl'):
             return FileKind.TBL
 
         if filename.endswith('.msg'):
             return FileKind.MSG
 
         error(filename, None, 'unknown file kind')
 
 
 def get_all_filenames():
-    """Get a list of all the files in the (Mercurial or Git) repository."""
+    '''Get a list of all the files in the (Mercurial or Git) repository.'''
     cmds = [['hg', 'manifest', '-q'], ['git', 'ls-files']]
     for cmd in cmds:
         try:
             all_filenames = subprocess.check_output(cmd, universal_newlines=True,
                                                     stderr=subprocess.PIPE).split('\n')
             return all_filenames
         except:
             continue
@@ -258,64 +291,208 @@ def check_style():
     ok = True
     for diffline in difflines:
         ok = False
         print(diffline, end='')
 
     return ok
 
 
+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', '')
+
+
+class Include(object):
+    '''Important information for a single #include statement.'''
+
+    def __init__(self, inclname, linenum, is_system):
+        self.inclname = inclname
+        self.linenum = linenum
+        self.is_system = is_system
+
+    def isLeaf(self):
+        return True
+
+    def section(self, module):
+        '''Identify which section inclname belongs to.
+
+        The section numbers are as follows.
+          0. Module header (e.g. jsfoo.h or jsfooinlines.h within jsfoo.cpp)
+          1. mozilla/Foo.h
+          2. <foo.h> or <foo>
+          3. jsfoo.h, prmjtime.h, etc
+          4. foo/Bar.h
+          5. jsfooinlines.h
+          6. foo/Bar-inl.h
+          7. non-.h, e.g. *.tbl, *.msg
+        '''
+
+        if self.is_system:
+            return 2
+
+        if not self.inclname.endswith('.h'):
+            return 7
+
+        # A couple of modules have the .h file in js/ and the .cpp file elsewhere and so need special
+        # handling.
+        if module == module_name(self.inclname) or \
+           module == 'jsmemorymetrics' and self.inclname == 'js/MemoryMetrics.h' or \
+           module == 'vm/PropertyKey' and self.inclname == 'js/PropertyKey.h':
+            return 0
+
+        if '/' in self.inclname:
+            if self.inclname.startswith('mozilla/'):
+                return 1
+
+            if self.inclname.endswith('-inl.h'):
+                return 6
+
+            return 4
+
+        if self.inclname.endswith('inlines.h'):
+            return 5
+
+        return 3
+
+    def quote(self):
+        if self.is_system:
+            return '<' + self.inclname + '>'
+        else:
+            return '"' + self.inclname + '"'
+
+
+class HashIfBlock(object):
+    '''Important information about a #if/#endif block.
+
+    A #if/#endif block is the contents of a #if/#endif (or similar) section.
+    The top-level block, which is not within a #if/#endif pair, is also
+    considered a block.
+
+    Each leaf is either an Include (representing a #include), or another
+    nested HashIfBlock.'''
+    def __init__(self):
+        self.kids = []
+
+    def isLeaf(self):
+        return False
+
+
 def do_file(filename, inclname, file_kind, f, all_inclnames, included_h_inclnames):
+    block_stack = [HashIfBlock()]
+
+    # Extract the #include statements as a tree of IBlocks and IIncludes.
     for linenum, line in enumerate(f, start=1):
         # Look for a |#include "..."| line.
         m = re.match(r'\s*#\s*include\s+"([^"]*)"', line)
         if m is not None:
-            included_inclname = m.group(1)
+            block_stack[-1].kids.append(Include(m.group(1), linenum, False))
+
+        # Look for a |#include <...>| line.
+        m = re.match(r'\s*#\s*include\s+<([^>]*)>', line)
+        if m is not None:
+            block_stack[-1].kids.append(Include(m.group(1), linenum, True))
+
+        # Look for a |#{if,ifdef,ifndef}| line.
+        m = re.match(r'\s*#\s*(if|ifdef|ifndef)\b', line)
+        if m is not None:
+            # Open a new block.
+            new_block = HashIfBlock()
+            block_stack[-1].kids.append(new_block)
+            block_stack.append(new_block)
 
-            if included_inclname not in included_inclnames_to_ignore:
-                included_kind = FileKind.get(included_inclname)
+        # Look for a |#{elif,else}| line.
+        m = re.match(r'\s*#\s*(elif|else)\b', line)
+        if m is not None:
+            # Close the current block, and open an adjacent one.
+            block_stack.pop()
+            new_block = HashIfBlock()
+            block_stack[-1].kids.append(new_block)
+            block_stack.append(new_block)
+
+        # Look for a |#endif| line.
+        m = re.match(r'\s*#\s*endif\b', line)
+        if m is not None:
+            # Close the current block.
+            block_stack.pop()
+
+    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).
+            if include.inclname in included_inclnames_to_ignore or \
+               include.inclname in all_inclnames:
+                error(filename, include.linenum,
+                      include.quote() + ' should be included using',
+                      'the #include "..." form')
+
+        else:
+            if include.inclname not in included_inclnames_to_ignore:
+                included_kind = FileKind.get(include.inclname)
 
                 # Check the #include path has the correct form.
-                if included_inclname not in all_inclnames:
-                    error(filename, linenum,
-                          '"' + included_inclname + '" is included ' + 'using the wrong path;',
+                if include.inclname not in all_inclnames:
+                    error(filename, include.linenum,
+                          include.quote() + ' is included ' + 'using the wrong path;',
                           'did you forget a prefix, or is the file not yet committed?')
 
                 # Record inclusions of .h files for cycle detection later.
                 # (Exclude .tbl and .msg files.)
                 elif included_kind == FileKind.H or included_kind == FileKind.INL_H:
-                    included_h_inclnames.add(included_inclname)
+                    included_h_inclnames.add(include.inclname)
 
                 # Check a H file doesn't #include an INL_H file.
                 if file_kind == FileKind.H and included_kind == FileKind.INL_H:
-                    error(filename, linenum,
-                          'vanilla header includes an inline-header file "' + included_inclname + '"')
+                    error(filename, include.linenum,
+                          'vanilla header includes an inline-header file ' + include.quote())
+
+                # Check a file doesn't #include itself.  (We do this here because the cycle
+                # detection below doesn't detect this case.)
+                if inclname == include.inclname:
+                    error(filename, include.linenum, 'the file includes itself')
 
-                # Check a file doesn't #include itself.  (We do this here because the
-                # cycle detection below doesn't detect this case.)
-                if inclname == included_inclname:
-                    error(filename, linenum, 'the file includes itself')
+    module = module_name(inclname)
+
+    def check_includes_order(include1, include2):
+        '''Check the ordering of two #include statements.'''
+
+        if include1.inclname in oddly_ordered_inclnames or \
+           include2.inclname in oddly_ordered_inclnames:
+            return
 
-        # Look for a |#include <...>| line.
-        m = re.match(r'\s*#\s*include\s+<([^>]*)>', line)
-        if m is not None:
-            included_inclname = m.group(1)
+        section1 = include1.section(module)
+        section2 = include2.section(module)
+        if (section1 > section2) or \
+           ((section1 == section2) and (include1.inclname.lower() > include2.inclname.lower())):
+            error(filename, str(include1.linenum) + ':' + str(include2.linenum),
+                  include1.quote() + ' should be included after ' + include2.quote())
+
+    # 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/', 'yarr/'))
 
-            # Check it is not a known local file (in which case it's
-            # probably a system header).
-            if included_inclname in included_inclnames_to_ignore or \
-               included_inclname in all_inclnames:
-                error(filename, linenum,
-                      '<' + included_inclname + '> should be included using',
-                      'the #include "..." form')
+    # Check the extracted #include statements, both individually, and the ordering of
+    # adjacent pairs that live in the same block.
+    def pair_traverse(prev, this):
+        if this.isLeaf():
+            check_include_statement(this)
+            if prev is not None and prev.isLeaf() and not skip_order_checking:
+                check_includes_order(prev, this)
+        else:
+            for prev2, this2 in zip([None] + this.kids[0:-1], this.kids):
+                pair_traverse(prev2, this2)
+
+    pair_traverse(None, block_stack[-1])
 
 
 def find_cycles(all_inclnames, edges):
-    """Find and draw any cycles."""
-    
+    '''Find and draw any cycles.'''
+
     SCCs = tarjan(all_inclnames, edges)
 
     # The various sorted() calls below ensure the output is deterministic.
 
     def draw_SCC(c):
         cset = set(c)
         drawn = set()
         def draw(v, indent):
@@ -387,10 +564,10 @@ def main():
     if ok:
         print('TEST-PASS | check_spidermonkey_style.py | ok')
     else:
         print('TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output;  diff is above')
 
     sys.exit(0 if ok else 1)
 
 
-if __name__ == "__main__":
+if __name__ == '__main__':
     main()
--- a/js/src/config/check_spidermonkey_style.py
+++ b/js/src/config/check_spidermonkey_style.py
@@ -5,37 +5,39 @@
 
 #----------------------------------------------------------------------------
 # This script checks various aspects of SpiderMonkey code style.  The current checks are as
 # follows.
 #
 # We check the following things in headers.
 #
 # - No cyclic dependencies.
-# 
+#
 # - No normal header should #include a inlines.h/-inl.h file.
-# 
+#
 # - #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.
-# 
+#
 # We check the following things in all files.
 #
 # - #includes should have full paths, e.g. "jit/Ion.h", not "Ion.h".
 #
 # - #includes should use the appropriate form for system headers (<...>) and
 #   local headers ("...").
 #
-# - #includes should be ordered correctly. (XXX: not yet implemented;  see bug
-#   888088)
+# - #includes should be ordered correctly.
 #   - Each one should be in the correct section.
 #   - Alphabetical order should be used within sections.
 #   - Sections should be in the right order.
+#   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 (including nested ones) in
+#   isolation, but don't try to do any order checking between such blocks.
 #----------------------------------------------------------------------------
 
 from __future__ import print_function
 
 import difflib
 import os
 import re
 import subprocess
@@ -82,43 +84,74 @@ included_inclnames_to_ignore = set([
     'unicode/udatpg.h',         # ICU
     'unicode/uenum.h',          # ICU
     'unicode/unum.h',           # ICU
     'unicode/ustring.h',        # ICU
     'unicode/utypes.h',         # ICU
     'vtune/VTuneWrapper.h'      # VTune
 ])
 
+# These files have additional constraints on where they are #included, so we
+# ignore #includes of them when checking #include ordering.
+oddly_ordered_inclnames = set([
+    'ctypes/typedefs.h',        # Included multiple times in the body of ctypes/CTypes.h
+    'jsautokw.h',               # Included in the body of frontend/TokenStream.h
+    'jswin.h',                  # Must be #included before <psapi.h>
+    'winbase.h',                # Must precede other system headers(?)
+    'windef.h'                  # Must precede other system headers(?)
+])
+
 # The files in tests/style/ contain code that fails this checking in various
 # ways.  Here is the output we expect.  If the actual output differs from
 # this, one of the following must have happened.
 # - New SpiderMonkey code violates one of the checked rules.
 # - The tests/style/ files have changed without expected_output being changed
 #   accordingly.
 # - This script has been broken somehow.
 #
 expected_output = '''\
 js/src/tests/style/BadIncludes2.h:1: error:
     vanilla header includes an inline-header file "tests/style/BadIncludes2-inl.h"
 
-js/src/tests/style/BadIncludes.h:1: error:
+js/src/tests/style/BadIncludes.h:3: error:
     the file includes itself
 
-js/src/tests/style/BadIncludes.h:3: error:
+js/src/tests/style/BadIncludes.h:6: error:
     "BadIncludes2.h" is included using the wrong path;
     did you forget a prefix, or is the file not yet committed?
 
-js/src/tests/style/BadIncludes.h:4: error:
+js/src/tests/style/BadIncludes.h:8: error:
     <tests/style/BadIncludes2.h> should be included using
     the #include "..." form
 
-js/src/tests/style/BadIncludes.h:5: error:
+js/src/tests/style/BadIncludes.h:10: error:
     "stdio.h" is included using the wrong path;
     did you forget a prefix, or is the file not yet committed?
 
+js/src/tests/style/BadIncludesOrder-inl.h:5:6: error:
+    "vm/Interpreter-inl.h" should be included after "jsscriptinlines.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:6:7: error:
+    "jsscriptinlines.h" should be included after "js/Value.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:7:8: error:
+    "js/Value.h" should be included after "ds/LifoAlloc.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:8:9: error:
+    "ds/LifoAlloc.h" should be included after "jsapi.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:9:10: error:
+    "jsapi.h" should be included after <stdio.h>
+
+js/src/tests/style/BadIncludesOrder-inl.h:10:11: error:
+    <stdio.h> should be included after "mozilla/HashFunctions.h"
+
+js/src/tests/style/BadIncludesOrder-inl.h:27:28: error:
+    "jsobj.h" should be included after "jsfun.h"
+
 (multiple files): error:
     header files form one or more cycles
 
    tests/style/HeaderCycleA1.h
    -> tests/style/HeaderCycleA2.h
       -> tests/style/HeaderCycleA3.h
          -> tests/style/HeaderCycleA1.h
 
@@ -138,18 +171,18 @@ actual_output = []
 
 def out(*lines):
     for line in lines:
         actual_output.append(line + '\n')
 
 
 def error(filename, linenum, *lines):
     location = filename
-    if linenum != None:
-        location += ":" + str(linenum)
+    if linenum is not None:
+        location += ':' + str(linenum)
     out(location + ': error:')
     for line in (lines):
         out('    ' + line)
     out('')
 
 
 class FileKind(object):
     C = 1
@@ -158,37 +191,37 @@ class FileKind(object):
     H = 4
     TBL = 5
     MSG = 6
 
     @staticmethod
     def get(filename):
         if filename.endswith('.c'):
             return FileKind.C
-       
+
         if filename.endswith('.cpp'):
             return FileKind.CPP
-       
+
         if filename.endswith(('inlines.h', '-inl.h')):
             return FileKind.INL_H
 
         if filename.endswith('.h'):
             return FileKind.H
 
         if filename.endswith('.tbl'):
             return FileKind.TBL
 
         if filename.endswith('.msg'):
             return FileKind.MSG
 
         error(filename, None, 'unknown file kind')
 
 
 def get_all_filenames():
-    """Get a list of all the files in the (Mercurial or Git) repository."""
+    '''Get a list of all the files in the (Mercurial or Git) repository.'''
     cmds = [['hg', 'manifest', '-q'], ['git', 'ls-files']]
     for cmd in cmds:
         try:
             all_filenames = subprocess.check_output(cmd, universal_newlines=True,
                                                     stderr=subprocess.PIPE).split('\n')
             return all_filenames
         except:
             continue
@@ -258,64 +291,208 @@ def check_style():
     ok = True
     for diffline in difflines:
         ok = False
         print(diffline, end='')
 
     return ok
 
 
+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', '')
+
+
+class Include(object):
+    '''Important information for a single #include statement.'''
+
+    def __init__(self, inclname, linenum, is_system):
+        self.inclname = inclname
+        self.linenum = linenum
+        self.is_system = is_system
+
+    def isLeaf(self):
+        return True
+
+    def section(self, module):
+        '''Identify which section inclname belongs to.
+
+        The section numbers are as follows.
+          0. Module header (e.g. jsfoo.h or jsfooinlines.h within jsfoo.cpp)
+          1. mozilla/Foo.h
+          2. <foo.h> or <foo>
+          3. jsfoo.h, prmjtime.h, etc
+          4. foo/Bar.h
+          5. jsfooinlines.h
+          6. foo/Bar-inl.h
+          7. non-.h, e.g. *.tbl, *.msg
+        '''
+
+        if self.is_system:
+            return 2
+
+        if not self.inclname.endswith('.h'):
+            return 7
+
+        # A couple of modules have the .h file in js/ and the .cpp file elsewhere and so need special
+        # handling.
+        if module == module_name(self.inclname) or \
+           module == 'jsmemorymetrics' and self.inclname == 'js/MemoryMetrics.h' or \
+           module == 'vm/PropertyKey' and self.inclname == 'js/PropertyKey.h':
+            return 0
+
+        if '/' in self.inclname:
+            if self.inclname.startswith('mozilla/'):
+                return 1
+
+            if self.inclname.endswith('-inl.h'):
+                return 6
+
+            return 4
+
+        if self.inclname.endswith('inlines.h'):
+            return 5
+
+        return 3
+
+    def quote(self):
+        if self.is_system:
+            return '<' + self.inclname + '>'
+        else:
+            return '"' + self.inclname + '"'
+
+
+class HashIfBlock(object):
+    '''Important information about a #if/#endif block.
+
+    A #if/#endif block is the contents of a #if/#endif (or similar) section.
+    The top-level block, which is not within a #if/#endif pair, is also
+    considered a block.
+
+    Each leaf is either an Include (representing a #include), or another
+    nested HashIfBlock.'''
+    def __init__(self):
+        self.kids = []
+
+    def isLeaf(self):
+        return False
+
+
 def do_file(filename, inclname, file_kind, f, all_inclnames, included_h_inclnames):
+    block_stack = [HashIfBlock()]
+
+    # Extract the #include statements as a tree of IBlocks and IIncludes.
     for linenum, line in enumerate(f, start=1):
         # Look for a |#include "..."| line.
         m = re.match(r'\s*#\s*include\s+"([^"]*)"', line)
         if m is not None:
-            included_inclname = m.group(1)
+            block_stack[-1].kids.append(Include(m.group(1), linenum, False))
+
+        # Look for a |#include <...>| line.
+        m = re.match(r'\s*#\s*include\s+<([^>]*)>', line)
+        if m is not None:
+            block_stack[-1].kids.append(Include(m.group(1), linenum, True))
+
+        # Look for a |#{if,ifdef,ifndef}| line.
+        m = re.match(r'\s*#\s*(if|ifdef|ifndef)\b', line)
+        if m is not None:
+            # Open a new block.
+            new_block = HashIfBlock()
+            block_stack[-1].kids.append(new_block)
+            block_stack.append(new_block)
 
-            if included_inclname not in included_inclnames_to_ignore:
-                included_kind = FileKind.get(included_inclname)
+        # Look for a |#{elif,else}| line.
+        m = re.match(r'\s*#\s*(elif|else)\b', line)
+        if m is not None:
+            # Close the current block, and open an adjacent one.
+            block_stack.pop()
+            new_block = HashIfBlock()
+            block_stack[-1].kids.append(new_block)
+            block_stack.append(new_block)
+
+        # Look for a |#endif| line.
+        m = re.match(r'\s*#\s*endif\b', line)
+        if m is not None:
+            # Close the current block.
+            block_stack.pop()
+
+    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).
+            if include.inclname in included_inclnames_to_ignore or \
+               include.inclname in all_inclnames:
+                error(filename, include.linenum,
+                      include.quote() + ' should be included using',
+                      'the #include "..." form')
+
+        else:
+            if include.inclname not in included_inclnames_to_ignore:
+                included_kind = FileKind.get(include.inclname)
 
                 # Check the #include path has the correct form.
-                if included_inclname not in all_inclnames:
-                    error(filename, linenum,
-                          '"' + included_inclname + '" is included ' + 'using the wrong path;',
+                if include.inclname not in all_inclnames:
+                    error(filename, include.linenum,
+                          include.quote() + ' is included ' + 'using the wrong path;',
                           'did you forget a prefix, or is the file not yet committed?')
 
                 # Record inclusions of .h files for cycle detection later.
                 # (Exclude .tbl and .msg files.)
                 elif included_kind == FileKind.H or included_kind == FileKind.INL_H:
-                    included_h_inclnames.add(included_inclname)
+                    included_h_inclnames.add(include.inclname)
 
                 # Check a H file doesn't #include an INL_H file.
                 if file_kind == FileKind.H and included_kind == FileKind.INL_H:
-                    error(filename, linenum,
-                          'vanilla header includes an inline-header file "' + included_inclname + '"')
+                    error(filename, include.linenum,
+                          'vanilla header includes an inline-header file ' + include.quote())
+
+                # Check a file doesn't #include itself.  (We do this here because the cycle
+                # detection below doesn't detect this case.)
+                if inclname == include.inclname:
+                    error(filename, include.linenum, 'the file includes itself')
 
-                # Check a file doesn't #include itself.  (We do this here because the
-                # cycle detection below doesn't detect this case.)
-                if inclname == included_inclname:
-                    error(filename, linenum, 'the file includes itself')
+    module = module_name(inclname)
+
+    def check_includes_order(include1, include2):
+        '''Check the ordering of two #include statements.'''
+
+        if include1.inclname in oddly_ordered_inclnames or \
+           include2.inclname in oddly_ordered_inclnames:
+            return
 
-        # Look for a |#include <...>| line.
-        m = re.match(r'\s*#\s*include\s+<([^>]*)>', line)
-        if m is not None:
-            included_inclname = m.group(1)
+        section1 = include1.section(module)
+        section2 = include2.section(module)
+        if (section1 > section2) or \
+           ((section1 == section2) and (include1.inclname.lower() > include2.inclname.lower())):
+            error(filename, str(include1.linenum) + ':' + str(include2.linenum),
+                  include1.quote() + ' should be included after ' + include2.quote())
+
+    # 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/', 'yarr/'))
 
-            # Check it is not a known local file (in which case it's
-            # probably a system header).
-            if included_inclname in included_inclnames_to_ignore or \
-               included_inclname in all_inclnames:
-                error(filename, linenum,
-                      '<' + included_inclname + '> should be included using',
-                      'the #include "..." form')
+    # Check the extracted #include statements, both individually, and the ordering of
+    # adjacent pairs that live in the same block.
+    def pair_traverse(prev, this):
+        if this.isLeaf():
+            check_include_statement(this)
+            if prev is not None and prev.isLeaf() and not skip_order_checking:
+                check_includes_order(prev, this)
+        else:
+            for prev2, this2 in zip([None] + this.kids[0:-1], this.kids):
+                pair_traverse(prev2, this2)
+
+    pair_traverse(None, block_stack[-1])
 
 
 def find_cycles(all_inclnames, edges):
-    """Find and draw any cycles."""
-    
+    '''Find and draw any cycles.'''
+
     SCCs = tarjan(all_inclnames, edges)
 
     # The various sorted() calls below ensure the output is deterministic.
 
     def draw_SCC(c):
         cset = set(c)
         drawn = set()
         def draw(v, indent):
@@ -387,10 +564,10 @@ def main():
     if ok:
         print('TEST-PASS | check_spidermonkey_style.py | ok')
     else:
         print('TEST-UNEXPECTED-FAIL | check_spidermonkey_style.py | actual output does not match expected output;  diff is above')
 
     sys.exit(0 if ok else 1)
 
 
-if __name__ == "__main__":
+if __name__ == '__main__':
     main()
--- a/js/src/tests/style/BadIncludes.h
+++ b/js/src/tests/style/BadIncludes.h
@@ -1,5 +1,11 @@
+// Note:  the #if/#elif conditions are to get past the #include order checking.
+#if A
 #include "tests/style/BadIncludes.h"    // bad: self-include
 #include "tests/style/BadIncludes2.h"   // ok
+#elif B
 #include "BadIncludes2.h"               // bad: not a full path
+#elif C
 #include <tests/style/BadIncludes2.h>   // bad: <> form used for local file
+#elif D
 #include "stdio.h"                      // bad: "" form used for system file
+#endif
new file mode 100644
--- /dev/null
+++ b/js/src/tests/style/BadIncludesOrder-inl.h
@@ -0,0 +1,30 @@
+// Note: Each #if scope gets checked separately.
+
+// These are in reverse order!
+#if A
+# include "vm/Interpreter-inl.h"
+# include "jsscriptinlines.h"
+# include "js/Value.h"
+# include "ds/LifoAlloc.h"
+# include "jsapi.h"
+# include <stdio.h>
+# include "mozilla/HashFunctions.h"
+#endif
+
+// These are in reverse order, but it's ok due to the #if scopes.
+#if B
+# include "vm/Interpreter-inl.h"
+# if C
+#  include "js/Value.h"
+#  if D
+#   include "jsapi.h"
+#  endif
+#  include <stdio.h>
+# endif
+# include "mozilla/HashFunctions.h"
+#endif
+
+#include "jsobj.h"
+#include "jsfun.h"      // out of order
+#include "jsscript.h"
+#include "jstypes.h"
--- a/js/src/tests/style/HeaderCycleB4-inl.h
+++ b/js/src/tests/style/HeaderCycleB4-inl.h
@@ -1,2 +1,2 @@
+#include "tests/style/jsheadercycleB5inlines.h"
 #include "tests/style/HeaderCycleB1-inl.h"
-#include "tests/style/jsheadercycleB5inlines.h"