From d7d2747c6c65f64fad7a040bb2a6a39f36a45602 Mon Sep 17 00:00:00 2001 From: "erg@google.com" Date: Wed, 7 Sep 2011 17:36:35 +0000 Subject: [PATCH] Update cpplint.py to #161: - ostream should be treated as a system header - Expand the MockCallback whitelist entry for gMock. - Virtual destructor check shouldn't get confused with "class EXPORT ClassName {" constructs. - Don't warn about the length of "$ Id: ... $" lines. - Better semicolon checks in for() loops. - Better whitespace comment checks. Review URL: http://codereview.appspot.com/4964064 --- cpplint/cpplint.py | 48 +++++++++++++++------- cpplint/cpplint_unittest.py | 79 ++++++++++++++++++++++++++++++++----- 2 files changed, 104 insertions(+), 23 deletions(-) diff --git a/cpplint/cpplint.py b/cpplint/cpplint.py index 86154ed..24fd40c 100755 --- a/cpplint/cpplint.py +++ b/cpplint/cpplint.py @@ -235,11 +235,11 @@ _CPP_HEADERS = frozenset([ 'cstdio', 'cstdlib', 'cstring', 'ctime', 'cwchar', 'cwctype', 'defalloc.h', 'deque.h', 'editbuf.h', 'exception', 'fstream', 'fstream.h', 'hashtable.h', 'heap.h', 'indstream.h', 'iomanip', - 'iomanip.h', 'ios', 'iosfwd', 'iostream', 'iostream.h', 'istream.h', - 'iterator.h', 'limits', 'map.h', 'multimap.h', 'multiset.h', - 'numeric', 'ostream.h', 'parsestream.h', 'pfstream.h', 'PlotFile.h', - 'procbuf.h', 'pthread_alloc.h', 'rope', 'rope.h', 'ropeimpl.h', - 'SFile.h', 'slist', 'slist.h', 'stack.h', 'stdexcept', + 'iomanip.h', 'ios', 'iosfwd', 'iostream', 'iostream.h', 'istream', + 'istream.h', 'iterator.h', 'limits', 'map.h', 'multimap.h', 'multiset.h', + 'numeric', 'ostream', 'ostream.h', 'parsestream.h', 'pfstream.h', + 'PlotFile.h', 'procbuf.h', 'pthread_alloc.h', 'rope', 'rope.h', + 'ropeimpl.h', 'SFile.h', 'slist', 'slist.h', 'stack.h', 'stdexcept', 'stdiostream.h', 'streambuf.h', 'stream.h', 'strfile.h', 'string', 'strstream', 'strstream.h', 'tempbuf.h', 'tree.h', 'typeinfo', 'valarray', ]) @@ -913,7 +913,7 @@ def CleanseComments(line): """ commentpos = line.find('//') if commentpos != -1 and not IsCppString(line[:commentpos]): - line = line[:commentpos] + line = line[:commentpos].rstrip() # get rid of /* ... */ return _RE_PATTERN_CLEANSE_LINE_C_COMMENTS.sub('', line) @@ -1378,11 +1378,14 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, # style guidelines, but it seems to perform well enough in testing # to be a worthwhile addition to the checks. classinfo_stack = class_state.classinfo_stack - # Look for a class declaration + # Look for a class declaration. The regexp accounts for decorated classes + # such as in: + # class LOCKABLE API Object { + # }; class_decl_match = Match( - r'\s*(template\s*<[\w\s<>,:]*>\s*)?(class|struct)\s+(\w+(::\w+)*)', line) + r'\s*(template\s*<[\w\s<>,:]*>\s*)?(class|struct)\s+([A-Z_]+\s+)*(\w+(::\w+)*)', line) if class_decl_match: - classinfo_stack.append(_ClassInfo(class_decl_match.group(3), linenum)) + classinfo_stack.append(_ClassInfo(class_decl_match.group(4), linenum)) # Everything else in this function uses the top of the stack if it's # not empty. @@ -1824,6 +1827,14 @@ def CheckSpacing(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/comma', 3, 'Missing space after ,') + # You should always have a space after a semicolon + # except for few corner cases + # TODO(unknown): clarify if 'if (1) { return 1;}' is requires one more + # space after ; + if Search(r';[^\s};\\)/]', line): + error(filename, linenum, 'whitespace/semicolon', 3, + 'Missing space after ;') + # Next we will look for issues with function calls. CheckSpacingForFunctionCall(filename, line, linenum, error) @@ -2119,8 +2130,12 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, error): # # URLs can be long too. It's possible to split these, but it makes them # harder to cut&paste. + # + # The "$Id:...$" comment may also get very long without it being the + # developers fault. if (not line.startswith('#include') and not is_header_guard and - not Match(r'^\s*//.*http(s?)://\S*$', line)): + not Match(r'^\s*//.*http(s?)://\S*$', line) and + not Match(r'^// \$Id:.*#[0-9]+ \$$', line)): line_width = GetLineWidth(line) if line_width > 100: error(filename, linenum, 'whitespace/line_length', 4, @@ -2402,9 +2417,12 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, include_state, 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. + # virtually indistinguishable from int(x) casts. Likewise, gMock's + # MockCallback takes a template parameter of the form return_type(arg_type), + # which looks much like the cast we're trying to detect. 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)): + not (Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line) or + Match(r'^\s*MockCallback<.*>', line))): error(filename, linenum, 'readability/casting', 4, 'Using deprecated casting style. ' 'Use static_cast<%s>(...) instead' % @@ -2634,15 +2652,17 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, # eg, void foo(int); or void foo(int) const; # The equals check is for function pointer assignment. # eg, void *(*foo)(int) = ... + # The > is for MockCallback<...> ... # # Right now, this will only catch cases where there's a single argument, and # it's unnamed. It should probably be expanded to check for multiple # arguments with some unnamed. - function_match = Match(r'\s*(\)|=|(const)?\s*(;|\{|throw\(\)))', remainder) + function_match = Match(r'\s*(\)|=|(const)?\s*(;|\{|throw\(\)|>))', remainder) if function_match: if (not function_match.group(3) or function_match.group(3) == ';' or - raw_line.find('/*') < 0): + ('MockCallback<' not in raw_line and + '/*' not in raw_line)): error(filename, linenum, 'readability/function', 3, 'All parameters should be named in a function') return diff --git a/cpplint/cpplint_unittest.py b/cpplint/cpplint_unittest.py index ea39f9c..e27573a 100755 --- a/cpplint/cpplint_unittest.py +++ b/cpplint/cpplint_unittest.py @@ -294,6 +294,13 @@ class CpplintTest(CpplintTestBase): self.TestLint( '// Read https://g' + ('o' * 60) + 'gle.com/' , '') + self.TestLint( + '// $Id: g' + ('o' * 80) + 'gle.cc#1 $', + '') + self.TestLint( + '// $Id: g' + ('o' * 80) + 'gle.cc#1', + 'Lines should be <= 80 characters long' + ' [whitespace/line_length] [2]') # Test error suppression annotations. def testErrorSuppression(self): @@ -490,6 +497,16 @@ class CpplintTest(CpplintTestBase): 'MOCK_CONST_METHOD2_T(method, double(float, float));', '') + # Like gMock method definitions, MockCallback instantiations look very similar + # to bad casts. + def testMockCallback(self): + self.TestLint( + 'MockCallback', + '') + self.TestLint( + 'MockCallback', + '') + # Test sizeof(type) cases. def testSizeofType(self): self.TestLint( @@ -737,7 +754,7 @@ class CpplintTest(CpplintTestBase): self.assertEquals((False, ''), f('a.cc', 'b.h')) def testCleanseLine(self): - self.assertEquals('int foo = 0; ', + self.assertEquals('int foo = 0;', cpplint.CleanseComments('int foo = 0; // danger!')) self.assertEquals('int o = 0;', cpplint.CleanseComments('int /* foo */ o = 0;')) @@ -1288,7 +1305,7 @@ class CpplintTest(CpplintTestBase): self.TestLint('void foo(const typename tm& tm);', '') # Returning an address of something is not prohibited. self.TestLint('return &something;', '') - self.TestLint('if (condition) {return &something;}', '') + self.TestLint('if (condition) {return &something; }', '') self.TestLint('if (condition) return &something;', '') self.TestLint('if (condition) address = &something;', '') self.TestLint('if (condition) result = lhs&rhs;', '') @@ -1317,11 +1334,29 @@ class CpplintTest(CpplintTestBase): def testSpacingForFncall(self): self.TestLint('if (foo) {', '') - self.TestLint('for (foo;bar;baz) {', '') + self.TestLint('for (foo; bar; baz) {', '') + self.TestLint('for (;;) {', '') + # Test that there is no warning when increment statement is empty. + self.TestLint('for (foo; baz;) {', '') + self.TestLint('for (foo;bar;baz) {', 'Missing space after ;' + ' [whitespace/semicolon] [3]') + # we don't warn about this semicolon, at least for now + self.TestLint('if (condition) {return &something; }', + '') + # seen in some macros + self.TestLint('DoSth();\\', '') + # Test that there is no warning about semicolon here. + self.TestLint('abc;// this is abc', + 'At least two spaces is best between code' + ' and comments [whitespace/comments] [2]') self.TestLint('while (foo) {', '') self.TestLint('switch (foo) {', '') self.TestLint('foo( bar)', 'Extra space after ( in function call' ' [whitespace/parens] [4]') + self.TestLint('foo( // comment', '') + self.TestLint('foo( // comment', + 'At least two spaces is best between code' + ' and comments [whitespace/comments] [2]') self.TestLint('foobar( \\', '') self.TestLint('foobar( \\', '') self.TestLint('( a + b)', 'Extra space after (' @@ -2204,22 +2239,26 @@ class CleansedLinesTest(unittest.TestCase): lines = ['Line 1', 'Line 2', 'Line 3 // Comment test', - 'Line 4 "foo"'] + 'Line 4 /* Comment test */', + 'Line 5 "foo"'] + clean_lines = cpplint.CleansedLines(lines) self.assertEquals(lines, clean_lines.raw_lines) - self.assertEquals(4, clean_lines.NumLines()) + self.assertEquals(5, clean_lines.NumLines()) self.assertEquals(['Line 1', 'Line 2', - 'Line 3 ', - 'Line 4 "foo"'], + 'Line 3', + 'Line 4', + 'Line 5 "foo"'], clean_lines.lines) self.assertEquals(['Line 1', 'Line 2', - 'Line 3 ', - 'Line 4 ""'], + 'Line 3', + 'Line 4', + 'Line 5 ""'], clean_lines.elided) def testInitEmpty(self): @@ -2804,6 +2843,28 @@ class NoNonVirtualDestructorsTest(CpplintTestBase): };''', '') + def testNoDestructorWhenVirtualNeededClassDecorated(self): + self.TestMultiLineLintRE( + '''class LOCKABLE API Foo { + virtual void foo(); + };''', + 'The class Foo probably needs a virtual destructor') + + def testDestructorNonVirtualWhenVirtualNeededClassDecorated(self): + self.TestMultiLineLintRE( + '''class LOCKABLE API Foo { + ~Foo(); + virtual void foo(); + };''', + 'The class Foo probably needs a virtual destructor') + + def testNoWarnWhenDerivedClassDecorated(self): + self.TestMultiLineLint( + '''class LOCKABLE API Foo : public Goo { + virtual void foo(); + };''', + '') + def testInternalBraces(self): self.TestMultiLineLintRE( '''class Foo {