From a868d2d7256663789f8ee6b943fa233bf558928a Mon Sep 17 00:00:00 2001 From: "erg@google.com" Date: Fri, 9 Oct 2009 21:18:45 +0000 Subject: [PATCH] Update cpplint.py to #131: - Optional check to make sure #includes are in alphabetical order. - Optional "--counting=" option for statistics on what errors were found. - Fix typos. - Warn on overloading the unary operator&(). (Binary operator&() is fine). - Fix false positives on "new int(x)"; it is not a cast. - Allow "NOLINT" on header guards. - Prevent members of a class from being "const string&". --- cpplint/cpplint.py | 146 +++++++++++++++++++++++++++++++----- cpplint/cpplint_unittest.py | 117 ++++++++++++++++++++++++----- 2 files changed, 227 insertions(+), 36 deletions(-) diff --git a/cpplint/cpplint.py b/cpplint/cpplint.py index bc46f09..807da6b 100755 --- a/cpplint/cpplint.py +++ b/cpplint/cpplint.py @@ -92,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 @@ -130,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. @@ -144,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 @@ -167,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 @@ -197,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 @@ -330,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. @@ -350,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: @@ -376,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 '' @@ -387,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) @@ -403,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. @@ -428,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() @@ -460,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 @@ -668,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)) @@ -931,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 @@ -939,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 @@ -1067,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) { @@ -1090,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*).') @@ -1154,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. @@ -1209,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 @@ -2149,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) @@ -2227,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', @@ -2323,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): @@ -2879,6 +2983,7 @@ def ParseArguments(args): """ try: (opts, filenames) = getopt.getopt(args, '', ['help', 'output=', 'verbose=', + 'counting=', 'filter=']) except getopt.GetoptError: PrintUsage('Invalid arguments.') @@ -2886,6 +2991,7 @@ def ParseArguments(args): verbosity = _VerboseLevel() output_format = _OutputFormat() filters = '' + counting_style = '' for (opt, val) in opts: if opt == '--help': @@ -2900,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.') @@ -2907,6 +3017,7 @@ def ParseArguments(args): _SetOutputFormat(output_format) _SetVerboseLevel(verbosity) _SetFilters(filters) + _SetCountingStyle(counting_style) return filenames @@ -2921,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/cpplint/cpplint_unittest.py b/cpplint/cpplint_unittest.py index 7fafa87..b574f78 100755 --- a/cpplint/cpplint_unittest.py +++ b/cpplint/cpplint_unittest.py @@ -438,6 +438,12 @@ class CpplintTest(CpplintTestBase): self.TestLint( 'operator bool(); // Conversion operator, o.k.', '') + self.TestLint( + 'new int64(123); // "new" operator on basic type, o.k.', + '') + self.TestLint( + 'new int64(123); // "new" operator on basic type, weird spacing', + '') # The second parameter to a gMock method definition is a function signature # that often looks like a bad cast but should not picked up by lint. @@ -976,6 +982,50 @@ class CpplintTest(CpplintTestBase): '' ' [runtime/printf] [4]') + # Test disallowed use of operator& and other operators. + def testIllegalOperatorOverloading(self): + errmsg = ('Unary operator& is dangerous. Do not use it.' + ' [runtime/operator] [4]') + self.TestLint('void operator=(const Myclass&)', '') + self.TestLint('void operator&(int a, int b)', '') # binary operator& ok + self.TestLint('void operator&() { }', errmsg) + self.TestLint('void operator & ( ) { }', + ['Extra space after ( [whitespace/parens] [2]', + errmsg + ]) + + # const string reference members are dangerous.. + def testConstStringReferenceMembers(self): + errmsg = ('const string& members are dangerous. It is much better to use ' + 'alternatives, such as pointers or simple constants.' + ' [runtime/member_string_references] [2]') + + members_declarations = ['const string& church', + 'const string &turing', + 'const string & godel'] + # TODO(unknown): Enable also these tests if and when we ever + # decide to check for arbitrary member references. + # "const Turing & a", + # "const Church& a", + # "const vector& a", + # "const Kurt::Godel & godel", + # "const Kazimierz::Kuratowski& kk" ] + + # The Good. + + self.TestLint('void f(const string&)', '') + self.TestLint('const string& f(const string& a, const string& b)', '') + self.TestLint('typedef const string& A;', '') + + for decl in members_declarations: + self.TestLint(decl + ' = b;', '') + self.TestLint(decl + ' =', '') + + # The Bad. + + for decl in members_declarations: + self.TestLint(decl + ';', errmsg) + # Variable-length arrays are not permitted. def testVariableLengthArrayDetection(self): errmsg = ('Do not use variable-length arrays. Use an appropriately named ' @@ -1904,6 +1954,27 @@ class CpplintTest(CpplintTestBase): ' [build/header_guard] [5]' % expected_guard), error_collector.ResultList()) + # incorrect header guard with nolint + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData(file_path, 'h', + ['#ifndef FOO // NOLINT', + '#define FOO', + '#endif // FOO NOLINT'], + error_collector) + self.assertEquals( + 0, + error_collector.ResultList().count( + '#ifndef header guard has wrong style, please use: %s' + ' [build/header_guard] [5]' % expected_guard), + error_collector.ResultList()) + self.assertEquals( + 0, + error_collector.ResultList().count( + '#endif line should be "#endif // %s"' + ' [build/header_guard] [5]' % expected_guard), + error_collector.ResultList()) + + def testBuildInclude(self): # Test that include statements have slashes in them. self.TestLint('#include "foo.h"', @@ -2137,7 +2208,6 @@ class CleansedLinesTest(unittest.TestCase): class OrderOfIncludesTest(CpplintTestBase): def setUp(self): self.include_state = cpplint._IncludeState() - # Cheat os.path.abspath called in FileInfo class. self.os_path_abspath_orig = os.path.abspath os.path.abspath = lambda value: value @@ -2254,24 +2324,6 @@ class OrderOfIncludesTest(CpplintTestBase): '"bar/bar-inl.h"', '"bar/bar.h"']), '') - # Test everything in a good but slightly deprecated order. - self.TestLanguageRulesCheck('foo/foo.cc', - Format(['"foo/foo.h"', - '', - '', - '"foo/foo-inl.h"', - '"bar/bar-inl.h"', - '"bar/bar.h"']), - '') - # Test everything in a good but deprecated order. - self.TestLanguageRulesCheck('foo/foo.cc', - Format(['', - '', - '"foo/foo-inl.h"', - '"bar/bar-inl.h"', - '"foo/foo.h"', - '"bar/bar.h"']), - '') # Test bad orders. self.TestLanguageRulesCheck( @@ -2302,6 +2354,21 @@ class OrderOfIncludesTest(CpplintTestBase): Format(['"foo/other/public/foo.h"', '']), '') + self.TestLanguageRulesCheck('foo/foo.cc', + Format(['"foo/foo.h"', + '', + '"base/google.h"', + '"base/flags.h"']), + 'Include "base/flags.h" not in alphabetical ' + 'order [build/include_alpha] [4]') + # According to the style, -inl.h should come before .h, but we don't + # complain about that. + self.TestLanguageRulesCheck('foo/foo.cc', + Format(['"foo/foo-inl.h"', + '"foo/foo.h"', + '"base/google.h"', + '"base/google-inl.h"']), + '') class CheckForFunctionLengthsTest(CpplintTestBase): @@ -2741,7 +2808,16 @@ 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]']) +# pylint: disable-msg=C6409 +def setUp(): + """ Runs before all tests are executed. + """ + # Enable all filters, so we don't miss anything that is off by default. + cpplint._DEFAULT_FILTERS = [] + cpplint._cpplint_state.SetFilters('') + +# pylint: disable-msg=C6409 def tearDown(): """A global check to make sure all error-categories have been tested. @@ -2756,6 +2832,7 @@ def tearDown(): # we assume we shouldn't run the test pass + if __name__ == '__main__': import sys # We don't want to run the VerifyAllCategoriesAreSeen() test unless @@ -2766,4 +2843,6 @@ if __name__ == '__main__': global _run_verifyallcategoriesseen _run_verifyallcategoriesseen = (len(sys.argv) == 1) + setUp() unittest.main() + tearDown()