Skip to content

Commit

Permalink
PRESUBMIT #include check enhancements.
Browse files Browse the repository at this point in the history 
1) Handle #define and #undef the same way as #if etc. See e.g.,
https://codereview.chromium.org/11363222/diff/10015/chrome/browser/history/history_unittest.cc
line 545.

2) Also headers can have the special first include, not only sources. See e.g.,
net/disk_cache/storage_block-inl.h.

NOTRY=true
BUG=NONE


Review URL: https://chromiumcodereview.appspot.com/11441035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@171511 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
marja@chromium.org committed Dec 6, 2012
1 parent 4aec5d3 commit ac294a1
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 38 deletions.
55 changes: 26 additions & 29 deletions PRESUBMIT.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,42 +542,42 @@ def _CheckIncludeOrderForScope(scope, input_api, file_path, changed_linenums):
return warnings


def _CheckIncludeOrderInFile(input_api, f, is_source, changed_linenums):
def _CheckIncludeOrderInFile(input_api, f, changed_linenums):
"""Checks the #include order for the given file f."""

system_include_pattern = input_api.re.compile(r'\s*#include \<.*')
# Exclude #include <.../...> includes from the check; e.g., <sys/...> includes
# often need to appear in a specific order.
excluded_include_pattern = input_api.re.compile(r'\s*#include \<.*/.*')
custom_include_pattern = input_api.re.compile(r'\s*#include "(?P<FILE>.*)"')
if_pattern = input_api.re.compile(r'\s*#\s*(if|elif|else|endif).*')
if_pattern = (
input_api.re.compile(r'\s*#\s*(if|elif|else|endif|define|undef).*'))

contents = f.NewContents()
warnings = []
line_num = 0

# Handle the special first include for source files. If the header file is
# some/path/file.h, the corresponding source file can be some/path/file.cc,
# some/other/path/file.cc, some/path/file_platform.cc etc. It's also possible
# that no special first include exists.
if is_source:
for line in contents:
line_num += 1
if system_include_pattern.match(line):
# Handle the special first include. If the first include file is
# some/path/file.h, the corresponding including file can be some/path/file.cc,
# some/other/path/file.cc, some/path/file_platform.cc, some/path/file-suffix.h
# etc. It's also possible that no special first include exists.
for line in contents:
line_num += 1
if system_include_pattern.match(line):
# No special first include -> process the line again along with normal
# includes.
line_num -= 1
break
match = custom_include_pattern.match(line)
if match:
match_dict = match.groupdict()
header_basename = input_api.os_path.basename(
match_dict['FILE']).replace('.h', '')
if header_basename not in input_api.os_path.basename(f.LocalPath()):
# No special first include -> process the line again along with normal
# includes.
line_num -= 1
break
match = custom_include_pattern.match(line)
if match:
match_dict = match.groupdict()
header_basename = input_api.os_path.basename(
match_dict['FILE']).replace('.h', '')
if header_basename not in input_api.os_path.basename(f.LocalPath()):
# No special first include -> process the line again along with normal
# includes.
line_num -= 1
break
break

# Split into scopes: Each region between #if and #endif is its own scope.
scopes = []
Expand Down Expand Up @@ -607,18 +607,15 @@ def _CheckIncludeOrder(input_api, output_api):
3. C++ system files in alphabetical order
4. Project's .h files in alphabetical order
Each region between #if and #endif follows these rules separately.
Each region separated by #if, #elif, #else, #endif, #define and #undef follows
these rules separately.
"""

warnings = []
for f in input_api.AffectedFiles():
changed_linenums = set([line_num for line_num, _ in f.ChangedContents()])
if f.LocalPath().endswith('.cc'):
warnings.extend(_CheckIncludeOrderInFile(input_api, f, True,
changed_linenums))
elif f.LocalPath().endswith('.h'):
warnings.extend(_CheckIncludeOrderInFile(input_api, f, False,
changed_linenums))
if f.LocalPath().endswith(('.cc', '.h')):
changed_linenums = set(line_num for line_num, _ in f.ChangedContents())
warnings.extend(_CheckIncludeOrderInFile(input_api, f, changed_linenums))

results = []
if warnings:
Expand Down
47 changes: 38 additions & 9 deletions PRESUBMIT_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,16 @@ def __init__(self, message, items=None, long_text=''):
self.long_text = long_text

class PresubmitError(PresubmitResult):
pass
def __init__(self, message, items, long_text=''):
MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)

class PresubmitPromptWarning(PresubmitResult):
pass
def __init__(self, message, items, long_text=''):
MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)

class PresubmitNotifyResult(PresubmitResult):
pass
def __init__(self, message, items, long_text=''):
MockOutputApi.PresubmitResult.__init__(self, message, items, long_text)


class MockFile(object):
Expand Down Expand Up @@ -120,7 +123,7 @@ def testSpecialFirstInclude1(self):
'#include "a/header.h"']
mock_file = MockFile('some/path/foo.cc', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, True, range(1, len(contents) + 1))
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(0, len(warnings))

def testSpecialFirstInclude2(self):
Expand All @@ -129,7 +132,7 @@ def testSpecialFirstInclude2(self):
'#include "a/header.h"']
mock_file = MockFile('some/path/foo.cc', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, True, range(1, len(contents) + 1))
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(0, len(warnings))

def testSpecialFirstInclude3(self):
Expand All @@ -138,7 +141,7 @@ def testSpecialFirstInclude3(self):
'#include "a/header.h"']
mock_file = MockFile('some/path/foo_platform.cc', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, True, range(1, len(contents) + 1))
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(0, len(warnings))

def testSpecialFirstInclude4(self):
Expand All @@ -147,10 +150,19 @@ def testSpecialFirstInclude4(self):
'#include "a/header.h"']
mock_file = MockFile('some/path/foo_platform.cc', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, True, range(1, len(contents) + 1))
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(1, len(warnings))
self.assertTrue('2' in warnings[0])

def testSpecialFirstInclude5(self):
mock_input_api = MockInputApi()
contents = ['#include "some/other/path/foo.h"',
'#include "a/header.h"']
mock_file = MockFile('some/path/foo-suffix.h', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(0, len(warnings))

def testOrderAlreadyWrong(self):
scope = [(1, '#include "b.h"'),
(2, '#include "a.h"'),
Expand Down Expand Up @@ -183,6 +195,10 @@ def testConflictAdded2(self):
def testIfElifElseEndif(self):
mock_input_api = MockInputApi()
contents = ['#include "e.h"',
'#define foo',
'#include "f.h"',
'#undef foo',
'#include "e.h"',
'#if foo',
'#include "d.h"',
'#elif bar',
Expand All @@ -193,7 +209,7 @@ def testIfElifElseEndif(self):
'#include "a.h"']
mock_file = MockFile('', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, True, range(1, len(contents) + 1))
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(0, len(warnings))

def testSysIncludes(self):
Expand All @@ -203,9 +219,22 @@ def testSysIncludes(self):
'#include <sys/a.h>']
mock_file = MockFile('', contents)
warnings = PRESUBMIT._CheckIncludeOrderInFile(
mock_input_api, mock_file, True, range(1, len(contents) + 1))
mock_input_api, mock_file, range(1, len(contents) + 1))
self.assertEqual(0, len(warnings))

def testCheckOnlyCFiles(self):
mock_input_api = MockInputApi()
mock_output_api = MockOutputApi()
contents = ['#include <b.h>',
'#include <a.h>']
mock_file_cc = MockFile('something.cc', contents)
mock_file_h = MockFile('something.h', contents)
mock_file_other = MockFile('something.py', contents)
mock_input_api.files = [mock_file_cc, mock_file_h, mock_file_other]
warnings = PRESUBMIT._CheckIncludeOrder(mock_input_api, mock_output_api)
self.assertEqual(1, len(warnings))
self.assertEqual(2, len(warnings[0].items))


class VersionControlerConflictsTest(unittest.TestCase):
def testTypicalConflict(self):
Expand Down

0 comments on commit ac294a1

Please sign in to comment.