From 26970fa9079112076ce036f3949806bbb2dac5fb Mon Sep 17 00:00:00 2001 From: "erg@google.com" Date: Tue, 17 Nov 2009 18:07:32 +0000 Subject: [PATCH] - Add a presubmit check that lints C++ files (will submit CLs that add this to PRESUBMIT.py in the chromium tree later). - Update cpplint.py to the latest version from the style guide. Review URL: http://codereview.chromium.org/395022 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@32180 0039d316-1c4b-4281-b951-d872f2087c98 --- cpplint.py | 178 +++++++++++++++++++++++++++++++----- presubmit_canned_checks.py | 46 +++++++++- tests/presubmit_unittest.py | 4 +- 3 files changed, 202 insertions(+), 26 deletions(-) diff --git a/cpplint.py b/cpplint.py index f7adef7dd..807da6b14 100755 --- a/cpplint.py +++ b/cpplint.py @@ -1,14 +1,32 @@ #!/usr/bin/python2.4 # -# cpplint.py is Copyright (C) 2009 Google Inc. +# Copyright (c) 2009 Google Inc. All rights reserved. # -# It is free software; you can redistribute it and/or modify it under the -# terms of either: +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: # -# a) the GNU General Public License as published by the Free Software -# Foundation; either version 1, or (at your option) any later version, or +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following disclaimer +# in the documentation and/or other materials provided with the +# distribution. +# * Neither the name of Google Inc. nor the names of its +# contributors may be used to endorse or promote products derived from +# this software without specific prior written permission. # -# b) the "Artistic License". +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. # Here are some issues that I've had people identify in my code during reviews, # that I think are possible to flag automatically in a lint tool. If these were @@ -74,6 +92,7 @@ import unicodedata _USAGE = """ Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] + [--counting=total|toplevel|detailed] [file] ... The style guidelines this tries to follow are those in @@ -112,6 +131,13 @@ Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] To see a list of all the categories used in cpplint, pass no arg: --filter= + + counting=total|toplevel|detailed + The total number of errors found is always printed. If + 'toplevel' is provided, then the count of errors in each of + the top-level categories like 'build' and 'whitespace' will + also be printed. If 'detailed' is provided, then a count + is provided for each category like 'build/class'. """ # We categorize each error message we print. Here are the categories. @@ -126,6 +152,7 @@ _ERROR_CATEGORIES = '''\ build/forward_decl build/header_guard build/include + build/include_alpha build/include_order build/include_what_you_use build/namespaces @@ -149,7 +176,9 @@ _ERROR_CATEGORIES = '''\ runtime/int runtime/init runtime/invalid_increment + runtime/member_string_references runtime/memset + runtime/operator runtime/printf runtime/printf_format runtime/references @@ -179,7 +208,7 @@ _ERROR_CATEGORIES = '''\ # 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 = [] +_DEFAULT_FILTERS = [ '-build/include_alpha' ] # 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 @@ -312,7 +341,40 @@ class _IncludeState(dict): def __init__(self): dict.__init__(self) + # The name of the current section. self._section = self._INITIAL_SECTION + # The path of last found header. + self._last_header = '' + + def CanonicalizeAlphabeticalOrder(self, header_path): + """Returns a path canonicalized for alphabetical comparisson. + + - replaces "-" with "_" so they both cmp the same. + - removes '-inl' since we don't require them to be after the main header. + - lowercase everything, just in case. + + Args: + header_path: Path to be canonicalized. + + Returns: + Canonicalized path. + """ + return header_path.replace('-inl.h', '.h').replace('-', '_').lower() + + def IsInAlphabeticalOrder(self, header_path): + """Check if a header is in alphabetical order with the previous header. + + Args: + header_path: Header to be checked. + + Returns: + Returns true if the header is in alphabetical order. + """ + canonical_header = self.CanonicalizeAlphabeticalOrder(header_path) + if self._last_header > canonical_header: + return False + self._last_header = canonical_header + return True def CheckNextIncludeOrder(self, header_type): """Returns a non-empty error message if the next header is out of order. @@ -332,15 +394,19 @@ class _IncludeState(dict): (self._TYPE_NAMES[header_type], self._SECTION_NAMES[self._section])) + last_section = self._section + if header_type == _C_SYS_HEADER: if self._section <= self._C_SECTION: self._section = self._C_SECTION else: + self._last_header = '' return error_message elif header_type == _CPP_SYS_HEADER: if self._section <= self._CPP_SECTION: self._section = self._CPP_SECTION else: + self._last_header = '' return error_message elif header_type == _LIKELY_MY_HEADER: if self._section <= self._MY_H_SECTION: @@ -358,6 +424,9 @@ class _IncludeState(dict): assert header_type == _OTHER_HEADER self._section = self._OTHER_H_SECTION + if last_section != self._section: + self._last_header = '' + return '' @@ -369,6 +438,8 @@ class _CppLintState(object): self.error_count = 0 # global count of reported errors # filters to apply when emitting error messages self.filters = _DEFAULT_FILTERS[:] + self.counting = 'total' # In what way are we counting errors? + self.errors_by_category = {} # string to int dict storing error counts # output format: # "emacs" - format that emacs can parse (default) @@ -385,6 +456,10 @@ class _CppLintState(object): self.verbose_level = level return last_verbose_level + def SetCountingStyle(self, counting_style): + """Sets the module's counting options.""" + self.counting = counting_style + def SetFilters(self, filters): """Sets the error-message filters. @@ -410,14 +485,27 @@ class _CppLintState(object): raise ValueError('Every filter in --filters must start with + or -' ' (%s does not)' % filt) - def ResetErrorCount(self): + def ResetErrorCounts(self): """Sets the module's error statistic back to zero.""" self.error_count = 0 + self.errors_by_category = {} - def IncrementErrorCount(self): + def IncrementErrorCount(self, category): """Bumps the module's error statistic.""" self.error_count += 1 - + if self.counting in ('toplevel', 'detailed'): + if self.counting != 'detailed': + category = category.split('/')[0] + if category not in self.errors_by_category: + self.errors_by_category[category] = 0 + self.errors_by_category[category] += 1 + + def PrintErrorCounts(self): + """Print a summary of errors by category, and the total.""" + for category, count in self.errors_by_category.iteritems(): + sys.stderr.write('Category \'%s\' errors found: %d\n' % + (category, count)) + sys.stderr.write('Total errors found: %d\n' % self.error_count) _cpplint_state = _CppLintState() @@ -442,6 +530,11 @@ def _SetVerboseLevel(level): return _cpplint_state.SetVerboseLevel(level) +def _SetCountingStyle(level): + """Sets the module's counting options.""" + _cpplint_state.SetCountingStyle(level) + + def _Filters(): """Returns the module's list of output filters, as a list.""" return _cpplint_state.filters @@ -650,7 +743,7 @@ def Error(filename, linenum, category, confidence, 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): - _cpplint_state.IncrementErrorCount() + _cpplint_state.IncrementErrorCount(category) if _cpplint_state.output_format == 'vs7': sys.stderr.write('%s(%s): %s [%s] [%d]\n' % ( filename, linenum, message, category, confidence)) @@ -913,7 +1006,7 @@ 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: + if ifndef != cppvar and not Search(r'\bNOLINT\b', lines[ifndef_linenum]): error_level = 0 if ifndef != cppvar + '_': error_level = 5 @@ -921,7 +1014,8 @@ def CheckForHeaderGuard(filename, lines, 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): + if (endif != ('#endif // %s' % cppvar) and + not Search(r'\bNOLINT\b', lines[endif_linenum])): error_level = 0 if endif != ('#endif // %s' % (cppvar + '_')): error_level = 5 @@ -1049,14 +1143,14 @@ def CheckPosixThreading(filename, clean_lines, linenum, error): '...) for improved thread safety.') -# Matches invalid increment: *count++, which moves pointer insead of +# Matches invalid increment: *count++, which moves pointer instead of # incrementing a value. -_RE_PATTERN_IVALID_INCREMENT = re.compile( +_RE_PATTERN_INVALID_INCREMENT = re.compile( r'^\s*\*\w+(\+\+|--);') def CheckInvalidIncrement(filename, clean_lines, linenum, error): - """Checks for invalud increment *count++. + """Checks for invalid increment *count++. For example following function: void increment_counter(int* count) { @@ -1072,7 +1166,7 @@ def CheckInvalidIncrement(filename, clean_lines, linenum, error): error: The function to call with any errors found. """ line = clean_lines.elided[linenum] - if _RE_PATTERN_IVALID_INCREMENT.match(line): + if _RE_PATTERN_INVALID_INCREMENT.match(line): error(filename, linenum, 'runtime/invalid_increment', 5, 'Changing pointer instead of value (or unused value of operator*).') @@ -1136,8 +1230,9 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, - classes with virtual methods need virtual destructors (compiler warning available, but not turned on yet.) - Additionally, check for constructor/destructor style violations as it - is very convenient to do so while checking for gcc-2 compliance. + Additionally, check for constructor/destructor style violations and reference + members, as it is very convenient to do so while checking for + gcc-2 compliance. Args: filename: The name of the current file. @@ -1191,6 +1286,18 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, error(filename, linenum, 'build/deprecated', 3, '>? and ))?' + # r'\s*const\s*' + type_name + '\s*&\s*\w+\s*;' + error(filename, linenum, 'runtime/member_string_references', 2, + 'const string& members are dangerous. It is much better to use ' + 'alternatives, such as pointers or simple constants.') + # Track class entry and exit, and attempt to find cases within the # class declaration that don't meet the C++ style # guidelines. Tracking is very dependent on the code matching Google @@ -2131,6 +2238,9 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): error(filename, linenum, 'build/include_order', 4, '%s. Should be: %s.h, c system, c++ system, other.' % (error_message, fileinfo.BaseName())) + if not include_state.IsInAlphabeticalOrder(include): + error(filename, linenum, 'build/include_alpha', 4, + 'Include "%s" not in alphabetical order' % include) # Look for any of the stream classes that are part of standard C++. match = _RE_PATTERN_INCLUDE.match(line) @@ -2209,16 +2319,18 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, # Parameterless conversion functions, such as bool(), are allowed as they are # probably a member operator declaration or default constructor. match = Search( - r'\b(int|float|double|bool|char|int32|uint32|int64|uint64)\([^)]', line) + r'(\bnew\s+)?\b' # Grab 'new' operator, if it's there + r'(int|float|double|bool|char|int32|uint32|int64|uint64)\([^)]', line) if match: # gMock methods are defined using some variant of MOCK_METHODx(name, type) # where type may be float(), int(string), etc. Without context they are # virtually indistinguishable from int(x) casts. - if not Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line): + if (match.group(1) is None and # If new operator, then this isn't a cast + not Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line)): error(filename, linenum, 'readability/casting', 4, 'Using deprecated casting style. ' 'Use static_cast<%s>(...) instead' % - match.group(1)) + match.group(2)) CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], 'static_cast', @@ -2305,6 +2417,16 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, error(filename, linenum, 'runtime/printf', 1, 'sscanf can be ok, but is slow and can overflow buffers.') + # Check if some verboten operator overloading is going on + # TODO(unknown): catch out-of-line unary operator&: + # class X {}; + # int operator&(const X& x) { return 42; } // unary operator& + # The trick is it's hard to tell apart from binary operator&: + # class Y { int operator&(const Y& x) { return 23; } }; // binary operator& + if Search(r'\boperator\s*&\s*\(\s*\)', line): + error(filename, linenum, 'runtime/operator', 4, + 'Unary operator& is dangerous. Do not use it.') + # Check for suspicious usage of "if" like # } if (a == b) { if Search(r'\}\s*if\s*\(', line): @@ -2861,6 +2983,7 @@ def ParseArguments(args): """ try: (opts, filenames) = getopt.getopt(args, '', ['help', 'output=', 'verbose=', + 'counting=', 'filter=']) except getopt.GetoptError: PrintUsage('Invalid arguments.') @@ -2868,6 +2991,7 @@ def ParseArguments(args): verbosity = _VerboseLevel() output_format = _OutputFormat() filters = '' + counting_style = '' for (opt, val) in opts: if opt == '--help': @@ -2882,6 +3006,10 @@ def ParseArguments(args): filters = val if not filters: PrintCategories() + elif opt == '--counting': + if val not in ('total', 'toplevel', 'detailed'): + PrintUsage('Valid counting options are total, toplevel, and detailed') + counting_style = val if not filenames: PrintUsage('No files were specified.') @@ -2889,6 +3017,7 @@ def ParseArguments(args): _SetOutputFormat(output_format) _SetVerboseLevel(verbosity) _SetFilters(filters) + _SetCountingStyle(counting_style) return filenames @@ -2903,10 +3032,11 @@ def main(): codecs.getwriter('utf8'), 'replace') - _cpplint_state.ResetErrorCount() + _cpplint_state.ResetErrorCounts() for filename in filenames: ProcessFile(filename, _cpplint_state.verbose_level) - sys.stderr.write('Total errors found: %d\n' % _cpplint_state.error_count) + _cpplint_state.PrintErrorCounts() + sys.exit(_cpplint_state.error_count > 0) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index f9088d783..3a5749007 100755 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -5,7 +5,6 @@ """Generic presubmit checks that can be reused by other presubmit checks.""" - ### Description checks def CheckChangeHasTestField(input_api, output_api): @@ -76,6 +75,51 @@ def CheckDoNotSubmitInFiles(input_api, output_api): return [] +def CheckChangeLintsClean(input_api, output_api, source_file_filter=None): + """Checks that all ".cc" and ".h" files pass cpplint.py.""" + _RE_IS_TEST = input_api.re.compile(r'.*tests?.(cc|h)$') + result = [] + + # Initialize cpplint. + import cpplint + cpplint._cpplint_state.ResetErrorCounts() + + # Justifications for each filter: + # + # - build/include : Too many; fix in the future. + # - build/include_order : Not happening; #ifdefed includes. + # - build/namespace : I'm surprised by how often we violate this rule. + # - readability/casting : Mistakes a whole bunch of function pointer. + # - runtime/int : Can be fixed long term; volume of errors too high + # - runtime/virtual : Broken now, but can be fixed in the future? + # - whitespace/braces : We have a lot of explicit scoping in chrome code. + cpplint._SetFilters("-build/include,-build/include_order,-build/namespace," + "-readability/casting,-runtime/int,-runtime/virtual," + "-whitespace/braces") + + # We currently are more strict with normal code than unit tests; 4 and 5 are + # the verbosity level that would normally be passed to cpplint.py through + # --verbose=#. Hopefully, in the future, we can be more verbose. + files = [f.AbsoluteLocalPath() for f in + input_api.AffectedSourceFiles(source_file_filter)] + for file_name in files: + if _RE_IS_TEST.match(file_name): + level = 5 + else: + level = 4 + + cpplint.ProcessFile(file_name, level) + + if cpplint._cpplint_state.error_count > 0: + if input_api.is_committing: + res_type = output_api.PresubmitError + else: + res_type = output_api.PresubmitPromptWarning + result = [res_type("Changelist failed cpplint.py check.")] + + return result + + def CheckChangeHasNoCR(input_api, output_api, source_file_filter=None): """Checks no '\r' (CR) character is in any source files.""" cr_files = [] diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 3149c9738..6ef523398 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1061,7 +1061,9 @@ class CannedChecksUnittest(PresubmitTestsBase): 'CheckChangeHasOnlyOneEol', 'CheckChangeHasNoCR', 'CheckChangeHasNoCrAndHasOnlyOneEol', 'CheckChangeHasNoTabs', 'CheckChangeHasQaField', 'CheckChangeHasTestedField', - 'CheckChangeHasTestField', 'CheckChangeSvnEolStyle', + 'CheckChangeHasTestField', + 'CheckChangeLintsClean', + 'CheckChangeSvnEolStyle', 'CheckSvnModifiedDirectories', 'CheckSvnForCommonMimeTypes', 'CheckSvnProperty', 'CheckDoNotSubmit',