StarMirror/styleguide
1
0
Fork 0
mirror of https://github.com/google/styleguide.git synced 2024-03-22 13:11:43 +08:00
Code Issues Projects Releases Wiki Activity

Update cpplint.py to #122:

Browse Source 
- Don't check quoted filenames with irrelevant tests.
- Make cpplint accept 'for (foo; bar; ) {}'.
- Work with temporary files generated by Emacs flymake-mode.
- Don't warn on "/// Doxygen comments."
- Check the use of DCHECK in the same way we check the use of CHECK.
- Properly handle relative file paths with IncludeWhatYouUse checking.
- Start checking for IncludeWhatYouUse in a limited way in .cc files.
This commit is contained in:
erg@google.com 2009-06-19 20:52:09 +00:00
parent a764d2becf
commit e35f765fa6
2 changed files with 330 additions and 40 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

226
cpplint/cpplint.py vendored
View File

@ -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 <functional>.
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: { '<functional>': (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]

144
cpplint/cpplint_unittest.py
View File

@ -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 <set>']
message = self.PerformIncludeWhatYouUse(
'''#include "blah/a.h"
std::set<int> 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<int> foo;''',
filename='blah/a_flymake.cc',
io=MockIo(mock_header_contents))
self.assertEquals(message, 'Add #include <set> 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<int> 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<int> foo;''' % os.path.basename(os.getcwd()),
filename='a.cc',
io=MockIo(mock_header_contents))
self.assertEquals(message, 'Add #include <set> 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 {',