From 0518964d220ff3e7be5d0356df58964685763a47 Mon Sep 17 00:00:00 2001 From: "erg+personal@google.com" Date: Fri, 30 Apr 2010 20:43:03 +0000 Subject: [PATCH] Update cpplint.py to #150: - Be explicit about "class Foo :\n public Bar" being wrong. - Allow snprintf(NULL, 0, ...) pattern. - Extend NOLINT syntax. - Remove NOLINT hint. - is an STL header. - Use original filename for header guard when invoked by flymake. - Avoid false-positive build/include_what_you_use errors for use of classes called "string" other than STL ones. Review URL: http://codereview.chromium.org/1697023 --- cpplint/cpplint.py | 246 +++++++++++++++++++++++------------- cpplint/cpplint_unittest.py | 76 +++++++++-- 2 files changed, 227 insertions(+), 95 deletions(-) diff --git a/cpplint/cpplint.py b/cpplint/cpplint.py index 4972f9d..a2d5bea 100755 --- a/cpplint/cpplint.py +++ b/cpplint/cpplint.py @@ -102,8 +102,9 @@ Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] certain of the problem, and 1 meaning it could be a legitimate construct. This will miss some errors, and is not a substitute for a code review. - To prevent specific lines from being linted, add a '// NOLINT' comment to the - end of the line. + To suppress false-positive errors of a certain category, add a + 'NOLINT(category)' comment to the line. NOLINT or NOLINT(*) + suppresses errors of all categories on that line. The files passed in will be linted; at least one file must be provided. Linted extensions are .cc, .cpp, and .h. Other file types will be ignored. @@ -145,64 +146,65 @@ Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] # If you add a new error message with a new category, add it to the list # here! cpplint_unittest.py should tell you if you forget to do this. # \ used for clearer layout -- pylint: disable-msg=C6013 -_ERROR_CATEGORIES = '''\ - build/class - build/deprecated - build/endif_comment - build/forward_decl - build/header_guard - build/include - build/include_alpha - build/include_order - build/include_what_you_use - build/namespaces - build/printf_format - build/storage_class - legal/copyright - readability/braces - readability/casting - readability/check - readability/constructors - readability/fn_size - readability/function - readability/multiline_comment - readability/multiline_string - readability/streams - readability/todo - readability/utf8 - runtime/arrays - runtime/casting - runtime/explicit - runtime/int - runtime/init - runtime/invalid_increment - runtime/member_string_references - runtime/memset - runtime/operator - runtime/printf - runtime/printf_format - runtime/references - runtime/rtti - runtime/sizeof - runtime/string - runtime/threadsafe_fn - runtime/virtual - whitespace/blank_line - whitespace/braces - whitespace/comma - whitespace/comments - whitespace/end_of_line - whitespace/ending_newline - whitespace/indent - whitespace/labels - whitespace/line_length - whitespace/newline - whitespace/operators - whitespace/parens - whitespace/semicolon - whitespace/tab - whitespace/todo -''' +_ERROR_CATEGORIES = [ + 'build/class', + 'build/deprecated', + 'build/endif_comment', + 'build/forward_decl', + 'build/header_guard', + 'build/include', + 'build/include_alpha', + 'build/include_order', + 'build/include_what_you_use', + 'build/namespaces', + 'build/printf_format', + 'build/storage_class', + 'legal/copyright', + 'readability/braces', + 'readability/casting', + 'readability/check', + 'readability/constructors', + 'readability/fn_size', + 'readability/function', + 'readability/multiline_comment', + 'readability/multiline_string', + 'readability/nolint', + 'readability/streams', + 'readability/todo', + 'readability/utf8', + 'runtime/arrays', + 'runtime/casting', + 'runtime/explicit', + 'runtime/int', + 'runtime/init', + 'runtime/invalid_increment', + 'runtime/member_string_references', + 'runtime/memset', + 'runtime/operator', + 'runtime/printf', + 'runtime/printf_format', + 'runtime/references', + 'runtime/rtti', + 'runtime/sizeof', + 'runtime/string', + 'runtime/threadsafe_fn', + 'runtime/virtual', + 'whitespace/blank_line', + 'whitespace/braces', + 'whitespace/comma', + 'whitespace/comments', + 'whitespace/end_of_line', + 'whitespace/ending_newline', + 'whitespace/indent', + 'whitespace/labels', + 'whitespace/line_length', + 'whitespace/newline', + 'whitespace/operators', + 'whitespace/parens', + 'whitespace/semicolon', + 'whitespace/tab', + '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 @@ -218,8 +220,8 @@ _DEFAULT_FILTERS = [ '-build/include_alpha' ] _STL_HEADERS = frozenset([ 'algobase.h', 'algorithm', 'alloc.h', 'bitset', 'deque', 'exception', 'function.h', 'functional', 'hash_map', 'hash_map.h', 'hash_set', - 'hash_set.h', 'iterator', 'list', 'list.h', 'map', 'memory', 'pair.h', - 'pthread_alloc', 'queue', 'set', 'set.h', 'sstream', 'stack', + 'hash_set.h', 'iterator', 'list', 'list.h', 'map', 'memory', 'new', + 'pair.h', 'pthread_alloc', 'queue', 'set', 'set.h', 'sstream', 'stack', 'stl_alloc.h', 'stl_relops.h', 'type_traits.h', 'utility', 'vector', 'vector.h', ]) @@ -287,6 +289,61 @@ _OTHER_HEADER = 5 _regexp_compile_cache = {} +# Finds occurrences of NOLINT or NOLINT(...). +_RE_SUPPRESSION = re.compile(r'\bNOLINT\b(\([^)]*\))?') + +# {str, set(int)}: a map from error categories to sets of linenumbers +# on which those errors are expected and should be suppressed. +_error_suppressions = {} + +def ParseNolintSuppressions(filename, raw_line, linenum, error): + """Updates the global list of error-suppressions. + + Parses any NOLINT comments on the current line, updating the global + error_suppressions store. Reports an error if the NOLINT comment + was malformed. + + Args: + filename: str, the name of the input file. + raw_line: str, the line of input text, with comments. + linenum: int, the number of the current line. + error: function, an error handler. + """ + # FIXME(adonovan): "NOLINT(" is misparsed as NOLINT(*). + m = _RE_SUPPRESSION.search(raw_line) + if m: + category = m.group(1) + if category in (None, '(*)'): # => "suppress all" + _error_suppressions.setdefault(None, set()).add(linenum) + else: + if category.startswith('(') and category.endswith(')'): + category = category[1:-1] + if category in _ERROR_CATEGORIES: + _error_suppressions.setdefault(category, set()).add(linenum) + else: + error(filename, linenum, 'readability/nolint', 5, + 'Unknown NOLINT error category: %s' % category) + + +def ResetNolintSuppressions(): + "Resets the set of NOLINT suppressions to empty." + _error_suppressions.clear() + + +def IsErrorSuppressedByNolint(category, linenum): + """Returns true if the specified error category is suppressed on this line. + + Consults the global error_suppressions map populated by + ParseNolintSuppressions/ResetNolintSuppressions. + + Args: + category: str, the category of the error. + linenum: int, the current line number. + Returns: + bool, True iff the error should be suppressed due to a NOLINT comment. + """ + return (linenum in _error_suppressions.get(category, set()) or + linenum in _error_suppressions.get(None, set())) def Match(pattern, s): """Matches the string with the pattern, caching the compiled regexp.""" @@ -701,10 +758,15 @@ class FileInfo: return self.Extension()[1:] in ('c', 'cc', 'cpp', 'cxx') -def _ShouldPrintError(category, confidence): - """Returns true iff confidence >= verbose, and category passes filter.""" - # There are two ways we might decide not to print an error message: +def _ShouldPrintError(category, confidence, linenum): + """Returns true iff confidence >= verbose, category passes + filter and is not NOLINT-suppressed.""" + + # There are three ways we might decide not to print an error message: + # a "NOLINT(category)" comment appears in the source, # the verbosity level isn't high enough, or the filters filter it out. + if IsErrorSuppressedByNolint(category, linenum): + return False if confidence < _cpplint_state.verbose_level: return False @@ -731,6 +793,10 @@ def Error(filename, linenum, category, confidence, message): that is, how certain we are this is a legitimate style regression, and not a misidentification or a use that's sometimes justified. + False positives can be suppressed by the use of + "cpplint(category)" comments on the offending line. These are + parsed into _error_suppressions. + Args: filename: The name of the file containing the error. linenum: The number of the line containing the error. @@ -742,9 +808,7 @@ def Error(filename, linenum, category, confidence, message): and 1 meaning that it could be a legitimate construct. message: The error message. """ - # There are two ways we might decide not to print an error message: - # the verbosity level isn't high enough, or the filters filter it out. - if _ShouldPrintError(category, confidence): + if _ShouldPrintError(category, confidence, linenum): _cpplint_state.IncrementErrorCount(category) if _cpplint_state.output_format == 'vs7': sys.stderr.write('%s(%s): %s [%s] [%d]\n' % ( @@ -962,6 +1026,10 @@ def GetHeaderGuardCPPVariable(filename): """ + # Restores original filename in case that cpplint is invoked from Emacs's + # flymake. + filename = re.sub(r'_flymake\.h$', '.h', filename) + fileinfo = FileInfo(filename) return re.sub(r'[-./\s]', '_', fileinfo.RepositoryName()).upper() + '_' @@ -1008,20 +1076,23 @@ def CheckForHeaderGuard(filename, lines, error): # The guard should be PATH_FILE_H_, but we also allow PATH_FILE_H__ # for backward compatibility. - if ifndef != cppvar and not Search(r'\bNOLINT\b', lines[ifndef_linenum]): + if ifndef != cppvar: error_level = 0 if ifndef != cppvar + '_': error_level = 5 + ParseNolintSuppressions(filename, lines[ifndef_linenum], ifndef_linenum, + error) error(filename, ifndef_linenum, 'build/header_guard', error_level, '#ifndef header guard has wrong style, please use: %s' % cppvar) - if (endif != ('#endif // %s' % cppvar) and - not Search(r'\bNOLINT\b', lines[endif_linenum])): + if endif != ('#endif // %s' % cppvar): error_level = 0 if endif != ('#endif // %s' % (cppvar + '_')): error_level = 5 + ParseNolintSuppressions(filename, lines[endif_linenum], endif_linenum, + error) error(filename, endif_linenum, 'build/header_guard', error_level, '#endif line should be "#endif // %s"' % cppvar) @@ -1519,8 +1590,7 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, error(filename, linenum, 'readability/fn_size', 5, 'Lint failed to find start of function body.') elif Match(r'^\}\s*$', line): # function end - if not Search(r'\bNOLINT\b', raw_line): - function_state.Check(error, filename, linenum) + function_state.Check(error, filename, linenum) function_state.End() elif not Match(r'^\s*$', line): function_state.Count() # Count non-blank/non-comment lines. @@ -2027,8 +2097,10 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, error): line): error(filename, linenum, 'whitespace/labels', 4, 'Labels should always be indented at least one space. ' - 'If this is a member-initializer list in a constructor, ' - 'the colon should be on the line after the definition header.') + 'If this is a member-initializer list in a constructor or ' + 'the base class list in a class definition, the colon should ' + 'be on the following line.') + # Check if the line is a header guard. is_header_guard = False @@ -2401,7 +2473,8 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, # When snprintf is used, the second argument shouldn't be a literal. match = Search(r'snprintf\s*\(([^,]*),\s*([0-9]*)\s*,', line) - if match: + if match and match.group(2) != '0': + # If 2nd arg is zero, snprintf is used to calculate size. error(filename, linenum, 'runtime/printf', 3, 'If you can, use sizeof(%s) instead of %s as the 2nd arg ' 'to snprintf.' % (match.group(1), match.group(2))) @@ -2750,8 +2823,13 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, continue # String is special -- it is a non-templatized type in STL. - if _RE_PATTERN_STRING.search(line): - required[''] = (linenum, 'string') + m = _RE_PATTERN_STRING.search(line) + if m: + # Don't warn about strings in non-STL namespaces: + # (We check only the first match per line; good enough.) + prefix = line[:m.start()] + if prefix.endswith('std::') or not prefix.endswith('::'): + required[''] = (linenum, 'string') for pattern, template, header in _re_pattern_algorithm_header: if pattern.search(line): @@ -2783,9 +2861,7 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, # 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' + abs_filename = re.sub(r'_flymake\.cc$', '.cc', abs_filename) # include_state is modified during iteration, so we iterate over a copy of # the keys. @@ -2836,9 +2912,8 @@ def ProcessLine(filename, file_extension, """ raw_lines = clean_lines.raw_lines + ParseNolintSuppressions(filename, raw_lines[line], line, error) CheckForFunctionLengths(filename, clean_lines, line, function_state, error) - if Search(r'\bNOLINT\b', raw_lines[line]): # ignore nolint lines - return CheckForMultilineCommentsAndStrings(filename, clean_lines, line, error) CheckStyle(filename, clean_lines, line, file_extension, error) CheckLanguage(filename, clean_lines, line, file_extension, include_state, @@ -2866,6 +2941,8 @@ def ProcessFileData(filename, file_extension, lines, error): function_state = _FunctionState() class_state = _ClassState() + ResetNolintSuppressions() + CheckForCopyright(filename, lines, error) if file_extension == 'h': @@ -2886,7 +2963,6 @@ def ProcessFileData(filename, file_extension, lines, error): CheckForNewlineAtEOF(filename, lines, error) - def ProcessFile(filename, vlevel): """Does google-lint on a single file. @@ -2968,7 +3044,7 @@ def PrintCategories(): These are the categories used to filter messages via --filter. """ - sys.stderr.write(_ERROR_CATEGORIES) + sys.stderr.write(''.join(' %s\n' % cat for cat in _ERROR_CATEGORIES)) sys.exit(0) diff --git a/cpplint/cpplint_unittest.py b/cpplint/cpplint_unittest.py index b574f78..7470f13 100755 --- a/cpplint/cpplint_unittest.py +++ b/cpplint/cpplint_unittest.py @@ -46,22 +46,22 @@ import cpplint # is in cpplint._ERROR_CATEGORIES, to help keep that list up to date. class ErrorCollector: # These are a global list, covering all categories seen ever. - _ERROR_CATEGORIES = [x.strip() # get rid of leading whitespace - for x in cpplint._ERROR_CATEGORIES.split()] + _ERROR_CATEGORIES = cpplint._ERROR_CATEGORIES _SEEN_ERROR_CATEGORIES = {} def __init__(self, assert_fn): """assert_fn: a function to call when we notice a problem.""" self._assert_fn = assert_fn self._errors = [] + cpplint.ResetNolintSuppressions() - def __call__(self, unused_filename, unused_linenum, + def __call__(self, unused_filename, linenum, category, confidence, message): self._assert_fn(category in self._ERROR_CATEGORIES, 'Message "%s" has category "%s",' ' which is not in _ERROR_CATEGORIES' % (message, category)) self._SEEN_ERROR_CATEGORIES[category] = 1 - if cpplint._ShouldPrintError(category, confidence): + if cpplint._ShouldPrintError(category, confidence, linenum): self._errors.append('%s [%s] [%d]' % (message, category, confidence)) def Results(self): @@ -295,6 +295,38 @@ class CpplintTest(CpplintTestBase): '// Read https://g' + ('o' * 60) + 'gle.com/' , '') + # Test error suppression annotations. + def testErrorSuppression(self): + # Two errors on same line: + self.TestLint( + 'long a = (int64) 65;', + ['Using C-style cast. Use static_cast(...) instead' + ' [readability/casting] [4]', + 'Use int16/int64/etc, rather than the C type long' + ' [runtime/int] [4]', + ]) + # One category of error suppressed: + self.TestLint( + 'long a = (int64) 65; // NOLINT(runtime/int)', + 'Using C-style cast. Use static_cast(...) instead' + ' [readability/casting] [4]') + # All categories suppressed: (two aliases) + self.TestLint('long a = (int64) 65; // NOLINT', '') + self.TestLint('long a = (int64) 65; // NOLINT(*)', '') + # Malformed NOLINT directive: + self.TestLint( + 'long a = 65; // NOLINT(foo)', + ['Unknown NOLINT error category: foo' + ' [readability/nolint] [5]', + 'Use int16/int64/etc, rather than the C type long [runtime/int] [4]', + ]) + # Irrelevant NOLINT directive has no effect: + self.TestLint( + 'long a = 65; // NOLINT(readability/casting)', + 'Use int16/int64/etc, rather than the C type long' + ' [runtime/int] [4]') + + # Test Variable Declarations. def testVariableDeclarations(self): self.TestLint( @@ -577,6 +609,12 @@ class CpplintTest(CpplintTestBase): self.TestIncludeWhatYouUse( 'void a(const string &foobar);', 'Add #include for string [build/include_what_you_use] [4]') + self.TestIncludeWhatYouUse( + 'void a(const std::string &foobar);', + 'Add #include for string [build/include_what_you_use] [4]') + self.TestIncludeWhatYouUse( + 'void a(const my::string &foobar);', + '') # Avoid false positives on strings in other namespaces. self.TestIncludeWhatYouUse( '''#include "base/foobar.h" bool foobar = swap(0,1); @@ -1627,9 +1665,9 @@ class CpplintTest(CpplintTestBase): def testLabel(self): self.TestLint('public:', 'Labels should always be indented at least one space. ' - 'If this is a member-initializer list in a constructor, ' - 'the colon should be on the line after the definition ' - 'header. [whitespace/labels] [4]') + 'If this is a member-initializer list in a constructor or ' + 'the base class list in a class definition, the colon should ' + 'be on the following line. [whitespace/labels] [4]') self.TestLint(' public:', '') self.TestLint(' public:', '') self.TestLint(' public:', '') @@ -1974,6 +2012,16 @@ class CpplintTest(CpplintTestBase): ' [build/header_guard] [5]' % expected_guard), error_collector.ResultList()) + # Special case for flymake + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData('mydir/foo_flymake.h', + 'h', [], error_collector) + self.assertEquals( + 1, + error_collector.ResultList().count( + 'No #ifndef header guard found, suggested CPP variable is: %s' + ' [build/header_guard] [5]' % expected_guard), + error_collector.ResultList()) def testBuildInclude(self): # Test that include statements have slashes in them. @@ -2728,9 +2776,10 @@ class NoNonVirtualDestructorsTest(CpplintTestBase): public Foo { virtual void goo(); };''', - 'Labels should always be indented at least one space. If this is a ' - 'member-initializer list in a constructor, the colon should be on the ' - 'line after the definition header. [whitespace/labels] [4]') + 'Labels should always be indented at least one space. ' + 'If this is a member-initializer list in a constructor or ' + 'the base class list in a class definition, the colon should ' + 'be on the following line. [whitespace/labels] [4]') def testNoDestructorWhenVirtualNeeded(self): self.TestMultiLineLintRE( @@ -2808,6 +2857,13 @@ class NoNonVirtualDestructorsTest(CpplintTestBase): 'The class Foo probably needs a virtual destructor due to having ' 'virtual method(s), one declared at line 2. [runtime/virtual] [4]']) + def testSnprintfSize(self): + self.TestLint('vsnprintf(NULL, 0, format)', '') + self.TestLint('snprintf(fisk, 1, format)', + 'If you can, use sizeof(fisk) instead of 1 as the 2nd arg ' + 'to snprintf. [runtime/printf] [3]') + + # pylint: disable-msg=C6409 def setUp(): """ Runs before all tests are executed.