diff --git a/cpplint/cpplint.py b/cpplint/cpplint.py index 182f246..f7adef7 100755 --- a/cpplint/cpplint.py +++ b/cpplint/cpplint.py @@ -175,6 +175,12 @@ _ERROR_CATEGORIES = '''\ whitespace/todo ''' +# The default state of the category filter. This is overrided by the --filter= +# flag. By default all errors are on, so only add here categories that should be +# off by default (i.e., categories that must be enabled by the --filter= flags). +# All entries here should start with a '-' or '+', as in the --filter= flag. +_DEFAULT_FILTERS = [] + # We used to check for high-bit characters, but after much discussion we # decided those were OK, as long as they were in UTF-8 and didn't represent # hard-coded international strings, which belong in a seperate i18n file. @@ -212,19 +218,20 @@ _CPP_HEADERS = frozenset([ # testing/base/gunit.h. Note that the _M versions need to come first # for substring matching to work. _CHECK_MACROS = [ - 'CHECK', + 'DCHECK', 'CHECK', 'EXPECT_TRUE_M', 'EXPECT_TRUE', 'ASSERT_TRUE_M', 'ASSERT_TRUE', 'EXPECT_FALSE_M', 'EXPECT_FALSE', 'ASSERT_FALSE_M', 'ASSERT_FALSE', ] -# Replacement macros for CHECK/EXPECT_TRUE/EXPECT_FALSE +# Replacement macros for CHECK/DCHECK/EXPECT_TRUE/EXPECT_FALSE _CHECK_REPLACEMENT = dict([(m, {}) for m in _CHECK_MACROS]) for op, replacement in [('==', 'EQ'), ('!=', 'NE'), ('>=', 'GE'), ('>', 'GT'), ('<=', 'LE'), ('<', 'LT')]: + _CHECK_REPLACEMENT['DCHECK'][op] = 'DCHECK_%s' % replacement _CHECK_REPLACEMENT['CHECK'][op] = 'CHECK_%s' % replacement _CHECK_REPLACEMENT['EXPECT_TRUE'][op] = 'EXPECT_%s' % replacement _CHECK_REPLACEMENT['ASSERT_TRUE'][op] = 'ASSERT_%s' % replacement @@ -360,7 +367,8 @@ class _CppLintState(object): def __init__(self): self.verbose_level = 1 # global setting. self.error_count = 0 # global count of reported errors - self.filters = [] # filters to apply when emitting error messages + # filters to apply when emitting error messages + self.filters = _DEFAULT_FILTERS[:] # output format: # "emacs" - format that emacs can parse (default) @@ -391,10 +399,12 @@ class _CppLintState(object): ValueError: The comma-separated filters did not all start with '+' or '-'. E.g. "-,+whitespace,-whitespace/indent,whitespace/badfilter" """ - if not filters: - self.filters = [] - else: - self.filters = filters.split(',') + # Default filters always have less priority than the flag ones. + self.filters = _DEFAULT_FILTERS[:] + for filt in filters.split(','): + clean_filt = filt.strip() + if clean_filt: + self.filters.append(clean_filt) for filt in self.filters: if not (filt.startswith('+') or filt.startswith('-')): raise ValueError('Every filter in --filters must start with + or -' @@ -1546,7 +1556,10 @@ def CheckSpacing(filename, clean_lines, linenum, error): # but some lines are exceptions -- e.g. if they're big # comment delimiters like: # //---------------------------------------------------------- - match = Search(r'[=/-]{4,}\s*$', line[commentend:]) + # or they begin with multiple slashes followed by a space: + # //////// Header comment + match = (Search(r'[=/-]{4,}\s*$', line[commentend:]) or + Search(r'^/+ ', line[commentend:])) if not match: error(filename, linenum, 'whitespace/comments', 4, 'Should have a space between // and comment') @@ -1607,14 +1620,15 @@ def CheckSpacing(filename, clean_lines, linenum, error): # consistent about how many spaces are inside the parens, and # there should either be zero or one spaces inside the parens. # We don't want: "if ( foo)" or "if ( foo )". - # Exception: "for ( ; foo; bar)" is allowed. + # Exception: "for ( ; foo; bar)" and "for (foo; bar; )" are allowed. match = Search(r'\b(if|for|while|switch)\s*' r'\(([ ]*)(.).*[^ ]+([ ]*)\)\s*{\s*$', line) if match: if len(match.group(2)) != len(match.group(4)): if not (match.group(3) == ';' and - len(match.group(2)) == 1 + len(match.group(4))): + len(match.group(2)) == 1 + len(match.group(4)) or + not match.group(2) and Search(r'\bfor\s*\(.*; \)', line)): error(filename, linenum, 'whitespace/parens', 5, 'Mismatching spaces inside () in %s' % match.group(1)) if not len(match.group(2)) in [0, 1]: @@ -2063,35 +2077,33 @@ def _ClassifyInclude(fileinfo, include, is_system): -def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, - error): - """Checks rules from the 'C++ language rules' section of cppguide.html. +def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): + """Check rules that are applicable to #include lines. - Some of these rules are hard to test (function overloading, using - uint32 inappropriately), but we do the best we can. + Strings on #include lines are NOT removed from elided line, to make + certain tasks easier. However, to prevent false positives, checks + applicable to #include lines in CheckLanguage must be put here. Args: filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - file_extension: The extension (without the dot) of the filename. include_state: An _IncludeState instance in which the headers are inserted. error: The function to call with any errors found. """ fileinfo = FileInfo(filename) - # get rid of comments - comment_elided_line = clean_lines.lines[linenum] + line = clean_lines.lines[linenum] # "include" should use the new style "foo/bar.h" instead of just "bar.h" - if _RE_PATTERN_INCLUDE_NEW_STYLE.search(comment_elided_line): + if _RE_PATTERN_INCLUDE_NEW_STYLE.search(line): error(filename, linenum, 'build/include', 4, 'Include the directory when naming .h files') # we shouldn't include a file more than once. actually, there are a # handful of instances where doing so is okay, but in general it's # not. - match = _RE_PATTERN_INCLUDE.search(comment_elided_line) + match = _RE_PATTERN_INCLUDE.search(line) if match: include = match.group(2) is_system = (match.group(1) == '<') @@ -2120,12 +2132,42 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, '%s. Should be: %s.h, c system, c++ system, other.' % (error_message, fileinfo.BaseName())) + # Look for any of the stream classes that are part of standard C++. + match = _RE_PATTERN_INCLUDE.match(line) + if match: + include = match.group(2) + if Match(r'(f|ind|io|i|o|parse|pf|stdio|str|)?stream$', include): + # Many unit tests use cout, so we exempt them. + if not _IsTestFilename(filename): + error(filename, linenum, 'readability/streams', 3, + 'Streams are highly discouraged.') + +def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, + error): + """Checks rules from the 'C++ language rules' section of cppguide.html. + + Some of these rules are hard to test (function overloading, using + uint32 inappropriately), but we do the best we can. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + file_extension: The extension (without the dot) of the filename. + include_state: An _IncludeState instance in which the headers are inserted. + error: The function to call with any errors found. + """ # If the line is empty or consists of entirely a comment, no need to # check it. line = clean_lines.elided[linenum] if not line: return + match = _RE_PATTERN_INCLUDE.search(line) + if match: + CheckIncludeLine(filename, clean_lines, linenum, include_state, error) + return + # Create an extended_line, which is the concatenation of the current and # next lines, for more effective checking of code that may span more than one # line. @@ -2139,16 +2181,6 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, # TODO(unknown): figure out if they're using default arguments in fn proto. - # Look for any of the stream classes that are part of standard C++. - match = _RE_PATTERN_INCLUDE.match(line) - if match: - include = match.group(2) - if Match(r'(f|ind|io|i|o|parse|pf|stdio|str|)?stream$', include): - # Many unit tests use cout, so we exempt them. - if not _IsTestFilename(filename): - error(filename, linenum, 'readability/streams', 3, - 'Streams are highly discouraged.') - # Check for non-const references in functions. This is tricky because & # is also used to take the address of something. We allow <> for templates, # (ignoring whatever is between the braces) and : for classes. @@ -2483,7 +2515,92 @@ for _header, _templates in _HEADERS_CONTAINING_TEMPLATES: _header)) -def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error): +def FilesBelongToSameModule(filename_cc, filename_h): + """Check if these two filenames belong to the same module. + + The concept of a 'module' here is a as follows: + foo.h, foo-inl.h, foo.cc, foo_test.cc and foo_unittest.cc belong to the + same 'module' if they are in the same directory. + some/path/public/xyzzy and some/path/internal/xyzzy are also considered + to belong to the same module here. + + If the filename_cc contains a longer path than the filename_h, for example, + '/absolute/path/to/base/sysinfo.cc', and this file would include + 'base/sysinfo.h', this function also produces the prefix needed to open the + header. This is used by the caller of this function to more robustly open the + header file. We don't have access to the real include paths in this context, + so we need this guesswork here. + + Known bugs: tools/base/bar.cc and base/bar.h belong to the same module + according to this implementation. Because of this, this function gives + some false positives. This should be sufficiently rare in practice. + + Args: + filename_cc: is the path for the .cc file + filename_h: is the path for the header path + + Returns: + Tuple with a bool and a string: + bool: True if filename_cc and filename_h belong to the same module. + string: the additional prefix needed to open the header file. + """ + + if not filename_cc.endswith('.cc'): + return (False, '') + filename_cc = filename_cc[:-len('.cc')] + if filename_cc.endswith('_unittest'): + filename_cc = filename_cc[:-len('_unittest')] + elif filename_cc.endswith('_test'): + filename_cc = filename_cc[:-len('_test')] + filename_cc = filename_cc.replace('/public/', '/') + filename_cc = filename_cc.replace('/internal/', '/') + + if not filename_h.endswith('.h'): + return (False, '') + filename_h = filename_h[:-len('.h')] + if filename_h.endswith('-inl'): + filename_h = filename_h[:-len('-inl')] + filename_h = filename_h.replace('/public/', '/') + filename_h = filename_h.replace('/internal/', '/') + + files_belong_to_same_module = filename_cc.endswith(filename_h) + common_path = '' + if files_belong_to_same_module: + common_path = filename_cc[:-len(filename_h)] + return files_belong_to_same_module, common_path + + +def UpdateIncludeState(filename, include_state, io=codecs): + """Fill up the include_state with new includes found from the file. + + Args: + filename: the name of the header to read. + include_state: an _IncludeState instance in which the headers are inserted. + io: The io factory to use to read the file. Provided for testability. + + Returns: + True if a header was succesfully added. False otherwise. + """ + headerfile = None + try: + headerfile = io.open(filename, 'r', 'utf8', 'replace') + except IOError: + return False + linenum = 0 + for line in headerfile: + linenum += 1 + clean_line = CleanseComments(line) + match = _RE_PATTERN_INCLUDE.search(clean_line) + if match: + include = match.group(2) + # The value formatting is cute, but not really used right now. + # What matters here is that the key is in include_state. + include_state.setdefault(include, '%s:%d' % (filename, linenum)) + return True + + +def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, + io=codecs): """Reports for missing stl includes. This function will output warnings to make sure you are including the headers @@ -2492,19 +2609,14 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error): less<> in a .h file, only one (the latter in the file) of these will be reported as a reason to include the . - We only check headers. We do not check inside cc-files. .cc files should be - able to depend on their respective header files for includes. However, there - is no simple way of producing this logic here. - Args: filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. include_state: An _IncludeState instance. error: The function to call with any errors found. + io: The IO factory to use to read the header file. Provided for unittest + injection. """ - if filename.endswith('.cc'): - return - required = {} # A map of header name to linenumber and the template entity. # Example of required: { '': (1219, 'less<>') } @@ -2529,6 +2641,44 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error): if pattern.search(line): required[header] = (linenum, template) + # The policy is that if you #include something in foo.h you don't need to + # include it again in foo.cc. Here, we will look at possible includes. + # Let's copy the include_state so it is only messed up within this function. + include_state = include_state.copy() + + # Did we find the header for this file (if any) and succesfully load it? + header_found = False + + # Use the absolute path so that matching works properly. + abs_filename = os.path.abspath(filename) + + # For Emacs's flymake. + # If cpplint is invoked from Emacs's flymake, a temporary file is generated + # by flymake and that file name might end with '_flymake.cc'. In that case, + # restore original file name here so that the corresponding header file can be + # found. + # e.g. If the file name is 'foo_flymake.cc', we should search for 'foo.h' + # instead of 'foo_flymake.h' + emacs_flymake_suffix = '_flymake.cc' + if abs_filename.endswith(emacs_flymake_suffix): + abs_filename = abs_filename[:-len(emacs_flymake_suffix)] + '.cc' + + # include_state is modified during iteration, so we iterate over a copy of + # the keys. + for header in include_state.keys(): #NOLINT + (same_module, common_path) = FilesBelongToSameModule(abs_filename, header) + fullpath = common_path + header + if same_module and UpdateIncludeState(fullpath, include_state, io): + header_found = True + + # If we can't find the header file for a .cc, assume it's because we don't + # know where to look. In that case we'll give up as we're not sure they + # didn't include it in the .h file. + # TODO(unknown): Do a better job of finding .h files so we are confident that + # not having the .h file means there isn't one. + if filename.endswith('.cc') and not header_found: + return + # All the lines have been processed, report the errors found. for required_header_unstripped in required: template = required[required_header_unstripped][1] diff --git a/cpplint/cpplint_unittest.py b/cpplint/cpplint_unittest.py index 1f6ed05..5b4b919 100755 --- a/cpplint/cpplint_unittest.py +++ b/cpplint/cpplint_unittest.py @@ -13,6 +13,9 @@ """Unit test for cpplint.py.""" +# TODO(unknown): Add a good test that tests UpdateIncludeState. + +import codecs import os import random import re @@ -72,6 +75,18 @@ class ErrorCollector: self._errors = self._errors[0:index] + self._errors[(index + 1):] break + +# This class is a lame mock of codecs. We do not verify filename, mode, or +# encoding, but for the current use case it is not needed. +class MockIo: + def __init__(self, mock_file): + self.mock_file = mock_file + + def open(self, unused_filename, unused_mode, unused_encoding, _): # NOLINT + # (lint doesn't like open as a method name) + return self.mock_file + + class CpplintTestBase(unittest.TestCase): """Provides some useful helper functions for cpplint tests.""" @@ -147,7 +162,7 @@ class CpplintTestBase(unittest.TestCase): function_state, error_collector) return error_collector.Results() - def PerformIncludeWhatYouUse(self, code, filename='foo.h'): + def PerformIncludeWhatYouUse(self, code, filename='foo.h', io=codecs): # First, build up the include state. error_collector = ErrorCollector(self.assert_) include_state = cpplint._IncludeState() @@ -163,7 +178,7 @@ class CpplintTestBase(unittest.TestCase): # Second, look for missing includes. cpplint.CheckForIncludeWhatYouUse(filename, lines, include_state, - error_collector) + error_collector, io) return error_collector.Results() # Perform lint and compare the error message with "expected_message". @@ -430,6 +445,15 @@ class CpplintTest(CpplintTestBase): 'Using sizeof(type). Use sizeof(varname) instead if possible' ' [runtime/sizeof] [1]') + # Test false errors that happened with some include file names + def testIncludeFilenameFalseError(self): + self.TestLint( + '#include "foo/long-foo.h"', + '') + self.TestLint( + '#include "foo/sprintf.h"', + '') + # Test typedef cases. There was a bug that cpplint misidentified # typedef for pointer to function as C-style cast and produced # false-positive error messages. @@ -581,6 +605,75 @@ class CpplintTest(CpplintTestBase): ''', '') + # Test the UpdateIncludeState code path. + mock_header_contents = ['#include "blah/foo.h"', '#include "blah/bar.h"'] + message = self.PerformIncludeWhatYouUse( + '#include "blah/a.h"', + filename='blah/a.cc', + io=MockIo(mock_header_contents)) + self.assertEquals(message, '') + + mock_header_contents = ['#include '] + message = self.PerformIncludeWhatYouUse( + '''#include "blah/a.h" + std::set foo;''', + filename='blah/a.cc', + io=MockIo(mock_header_contents)) + self.assertEquals(message, '') + + # Make sure we can find the correct header file if the cc file seems to be + # a temporary file generated by Emacs's flymake. + mock_header_contents = [''] + message = self.PerformIncludeWhatYouUse( + '''#include "blah/a.h" + std::set foo;''', + filename='blah/a_flymake.cc', + io=MockIo(mock_header_contents)) + self.assertEquals(message, 'Add #include for set<> ' + '[build/include_what_you_use] [4]') + + # If there's just a cc and the header can't be found then it's ok. + message = self.PerformIncludeWhatYouUse( + '''#include "blah/a.h" + std::set foo;''', + filename='blah/a.cc') + self.assertEquals(message, '') + + # Make sure we find the headers with relative paths. + mock_header_contents = [''] + message = self.PerformIncludeWhatYouUse( + '''#include "%s/a.h" + std::set foo;''' % os.path.basename(os.getcwd()), + filename='a.cc', + io=MockIo(mock_header_contents)) + self.assertEquals(message, 'Add #include for set<> ' + '[build/include_what_you_use] [4]') + + def testFilesBelongToSameModule(self): + f = cpplint.FilesBelongToSameModule + self.assertEquals((True, ''), f('a.cc', 'a.h')) + self.assertEquals((True, ''), f('base/google.cc', 'base/google.h')) + self.assertEquals((True, ''), f('base/google_test.cc', 'base/google.h')) + self.assertEquals((True, ''), + f('base/google_unittest.cc', 'base/google.h')) + self.assertEquals((True, ''), + f('base/internal/google_unittest.cc', + 'base/public/google.h')) + self.assertEquals((True, 'xxx/yyy/'), + f('xxx/yyy/base/internal/google_unittest.cc', + 'base/public/google.h')) + self.assertEquals((True, 'xxx/yyy/'), + f('xxx/yyy/base/google_unittest.cc', + 'base/public/google.h')) + self.assertEquals((True, ''), + f('base/google_unittest.cc', 'base/google-inl.h')) + self.assertEquals((True, '/home/build/google3/'), + f('/home/build/google3/base/google.cc', 'base/google.h')) + + self.assertEquals((False, ''), + f('/home/build/google3/base/google.cc', 'basu/google.h')) + self.assertEquals((False, ''), f('a.cc', 'b.h')) + def testCleanseLine(self): self.assertEquals('int foo = 0; ', cpplint.CleanseComments('int foo = 0; // danger!')) @@ -962,6 +1055,25 @@ class CpplintTest(CpplintTestBase): 'Consider using CHECK_LT instead of CHECK(a < b)' ' [readability/check] [2]') + self.TestLint('DCHECK(x == 42)', + 'Consider using DCHECK_EQ instead of DCHECK(a == b)' + ' [readability/check] [2]') + self.TestLint('DCHECK(x != 42)', + 'Consider using DCHECK_NE instead of DCHECK(a != b)' + ' [readability/check] [2]') + self.TestLint('DCHECK(x >= 42)', + 'Consider using DCHECK_GE instead of DCHECK(a >= b)' + ' [readability/check] [2]') + self.TestLint('DCHECK(x > 42)', + 'Consider using DCHECK_GT instead of DCHECK(a > b)' + ' [readability/check] [2]') + self.TestLint('DCHECK(x <= 42)', + 'Consider using DCHECK_LE instead of DCHECK(a <= b)' + ' [readability/check] [2]') + self.TestLint('DCHECK(x < 42)', + 'Consider using DCHECK_LT instead of DCHECK(a < b)' + ' [readability/check] [2]') + self.TestLint( 'EXPECT_TRUE("42" == x)', 'Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)' @@ -1088,9 +1200,12 @@ class CpplintTest(CpplintTestBase): ' [whitespace/parens] [5]') self.TestLint('switch ( foo) {', 'Mismatching spaces inside () in switch' ' [whitespace/parens] [5]') + self.TestLint('for (foo; ba; bar ) {', 'Mismatching spaces inside () in for' + ' [whitespace/parens] [5]') self.TestLint('for (; foo; bar) {', '') self.TestLint('for ( ; foo; bar) {', '') self.TestLint('for ( ; foo; bar ) {', '') + self.TestLint('for (foo; bar; ) {', '') self.TestLint('while ( foo ) {', 'Should have zero or one spaces inside' ' ( and ) in while [whitespace/parens] [5]') @@ -1285,6 +1400,13 @@ class CpplintTest(CpplintTestBase): self.TestLint('//x', 'Should have a space between // and comment' ' [whitespace/comments] [4]') self.TestLint('// x', '') + self.TestLint('//----', '') + self.TestLint('//====', '') + self.TestLint('//////', '') + self.TestLint('////// x', '') + self.TestLint('/// x', '') + self.TestLint('////x', 'Should have a space between // and comment' + ' [whitespace/comments] [4]') # Test a line preceded by empty or comment lines. There was a bug # that caused it to print the same warning N times if the erroneous @@ -1529,6 +1651,24 @@ class CpplintTest(CpplintTestBase): finally: cpplint._cpplint_state.filters = old_filters + def testDefaultFilter(self): + default_filters = cpplint._DEFAULT_FILTERS + old_filters = cpplint._cpplint_state.filters + cpplint._DEFAULT_FILTERS = [ '-whitespace' ] + try: + # Reset filters + cpplint._cpplint_state.SetFilters('') + self.TestLint('// Hello there ', '') + cpplint._cpplint_state.SetFilters('+whitespace/end_of_line') + self.TestLint( + '// Hello there ', + 'Line ends in whitespace. Consider deleting these extra spaces.' + ' [whitespace/end_of_line] [4]') + self.TestLint(' weird opening space', '') + finally: + cpplint._cpplint_state.filters = old_filters + cpplint._DEFAULT_FILTERS = default_filters + def testUnnamedNamespacesInHeaders(self): self.TestLanguageRulesCheck( 'foo.h', 'namespace {',