Index: cpplint.py |
diff --git a/cpplint.py b/cpplint.py |
old mode 100755 |
new mode 100644 |
index 62f475b0d6cf2807c0762440c732e6c599e2e0ca..f0416f20b18c3e50c7665175433e263e5215b409 |
--- a/cpplint.py |
+++ b/cpplint.py |
@@ -194,6 +194,7 @@ _ERROR_CATEGORIES = [ |
'readability/constructors', |
'readability/fn_size', |
'readability/function', |
+ 'readability/inheritance', |
'readability/multiline_comment', |
'readability/multiline_string', |
'readability/namespace', |
@@ -210,6 +211,7 @@ _ERROR_CATEGORIES = [ |
'runtime/invalid_increment', |
'runtime/member_string_references', |
'runtime/memset', |
+ 'runtime/indentation_namespace', |
'runtime/operator', |
'runtime/printf', |
'runtime/printf_format', |
@@ -383,6 +385,15 @@ _CPP_HEADERS = frozenset([ |
]) |
+# These headers are excluded from [build/include] and [build/include_order] |
+# checks: |
+# - Anything not following google file name conventions (containing an |
+# uppercase character, such as Python.h or nsStringAPI.h, for example). |
+# - Lua headers. |
+_THIRD_PARTY_HEADERS_PATTERN = re.compile( |
+ r'^(?:[^/]*[A-Z][^/]*\.h|lua\.h|lauxlib\.h|lualib\.h)$') |
+ |
+ |
# Assertion macros. These are defined in base/logging.h and |
# testing/base/gunit.h. Note that the _M versions need to come first |
# for substring matching to work. |
@@ -465,9 +476,6 @@ _MATCH_ASM = re.compile(r'^\s*(?:asm|_asm|__asm|__asm__)' |
_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 = {} |
@@ -497,24 +505,27 @@ def ParseNolintSuppressions(filename, raw_line, linenum, error): |
linenum: int, the number of the current line. |
error: function, an error handler. |
""" |
- # FIXME(adonovan): "NOLINT(" is misparsed as NOLINT(*). |
- matched = _RE_SUPPRESSION.search(raw_line) |
+ matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line) |
if matched: |
- category = matched.group(1) |
+ if matched.group(1): |
+ suppressed_line = linenum + 1 |
+ else: |
+ suppressed_line = linenum |
+ category = matched.group(2) |
if category in (None, '(*)'): # => "suppress all" |
- _error_suppressions.setdefault(None, set()).add(linenum) |
+ _error_suppressions.setdefault(None, set()).add(suppressed_line) |
else: |
if category.startswith('(') and category.endswith(')'): |
category = category[1:-1] |
if category in _ERROR_CATEGORIES: |
- _error_suppressions.setdefault(category, set()).add(linenum) |
+ _error_suppressions.setdefault(category, set()).add(suppressed_line) |
else: |
error(filename, linenum, 'readability/nolint', 5, |
'Unknown NOLINT error category: %s' % category) |
def ResetNolintSuppressions(): |
- "Resets the set of NOLINT suppressions to empty." |
+ """Resets the set of NOLINT suppressions to empty.""" |
_error_suppressions.clear() |
@@ -569,11 +580,12 @@ def Search(pattern, s): |
return _regexp_compile_cache[pattern].search(s) |
-class _IncludeState(dict): |
+class _IncludeState(object): |
"""Tracks line numbers for includes, and the order in which includes appear. |
- As a dict, an _IncludeState object serves as a mapping between include |
- filename and line number on which that file was included. |
+ include_list contains list of lists of (header, line number) pairs. |
+ It's a lists of lists rather than just one flat list to make it |
+ easier to update across preprocessor boundaries. |
Call CheckNextIncludeOrder() once for each header in the file, passing |
in the type constants defined above. Calls in an illegal order will |
@@ -604,15 +616,42 @@ class _IncludeState(dict): |
} |
def __init__(self): |
- dict.__init__(self) |
- self.ResetSection() |
+ self.include_list = [[]] |
+ self.ResetSection('') |
+ |
+ def FindHeader(self, header): |
+ """Check if a header has already been included. |
+ |
+ Args: |
+ header: header to check. |
+ Returns: |
+ Line number of previous occurrence, or -1 if the header has not |
+ been seen before. |
+ """ |
+ for section_list in self.include_list: |
+ for f in section_list: |
+ if f[0] == header: |
+ return f[1] |
+ return -1 |
+ |
+ def ResetSection(self, directive): |
+ """Reset section checking for preprocessor directive. |
- def ResetSection(self): |
+ Args: |
+ directive: preprocessor directive (e.g. "if", "else"). |
+ """ |
# The name of the current section. |
self._section = self._INITIAL_SECTION |
# The path of last found header. |
self._last_header = '' |
+ # Update list of includes. Note that we never pop from the |
+ # include list. |
+ if directive in ('if', 'ifdef', 'ifndef'): |
+ self.include_list.append([]) |
+ elif directive in ('else', 'elif'): |
+ self.include_list[-1] = [] |
+ |
def SetLastHeader(self, header_path): |
self._last_header = header_path |
@@ -844,7 +883,7 @@ def _SetFilters(filters): |
def _AddFilters(filters): |
"""Adds more filter overrides. |
- |
+ |
Unlike _SetFilters, this function does not reset the current list of filters |
available. |
@@ -923,7 +962,7 @@ class _IncludeError(Exception): |
pass |
-class FileInfo: |
+class FileInfo(object): |
"""Provides utility functions for filenames. |
FileInfo provides easy access to the components of a file's path |
@@ -1634,6 +1673,16 @@ def CheckForHeaderGuard(filename, lines, error): |
error: The function to call with any errors found. |
""" |
+ # Don't check for header guards if there are error suppression |
+ # comments somewhere in this file. |
+ # |
+ # Because this is silencing a warning for a nonexistent line, we |
+ # only support the very specific NOLINT(build/header_guard) syntax, |
+ # and not the general NOLINT or NOLINT(*) syntax. |
+ for i in lines: |
+ if Search(r'//\s*NOLINT\(build/header_guard\)', i): |
+ return |
+ |
cppvar = GetHeaderGuardCPPVariable(filename) |
ifndef = None |
@@ -1880,6 +1929,20 @@ def CheckInvalidIncrement(filename, clean_lines, linenum, error): |
'Changing pointer instead of value (or unused value of operator*).') |
+def IsMacroDefinition(clean_lines, linenum): |
+ if Search(r'^#define', clean_lines[linenum]): |
+ return True |
+ |
+ if linenum > 0 and Search(r'\\$', clean_lines[linenum - 1]): |
+ return True |
+ |
+ return False |
+ |
+ |
+def IsForwardClassDeclaration(clean_lines, linenum): |
+ return Match(r'^\s*(\btemplate\b)*.*class\s+\w+;\s*$', clean_lines[linenum]) |
+ |
+ |
class _BlockInfo(object): |
"""Stores information about a generic block of code.""" |
@@ -1887,6 +1950,7 @@ class _BlockInfo(object): |
self.seen_open_brace = seen_open_brace |
self.open_parentheses = 0 |
self.inline_asm = _NO_ASM |
+ self.check_namespace_indentation = False |
def CheckBegin(self, filename, clean_lines, linenum, error): |
"""Run checks that applies to text up to the opening brace. |
@@ -1943,6 +2007,7 @@ class _ClassInfo(_BlockInfo): |
self.name = name |
self.starting_linenum = linenum |
self.is_derived = False |
+ self.check_namespace_indentation = True |
if class_or_struct == 'struct': |
self.access = 'public' |
self.is_struct = True |
@@ -1994,6 +2059,7 @@ class _NamespaceInfo(_BlockInfo): |
_BlockInfo.__init__(self, False) |
self.name = name or '' |
self.starting_linenum = linenum |
+ self.check_namespace_indentation = True |
def CheckEnd(self, filename, clean_lines, linenum, error): |
"""Check end of namespace comments.""" |
@@ -2531,17 +2597,73 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, |
base_classname = classinfo.name.split('::')[-1] |
# Look for single-argument constructors that aren't marked explicit. |
- # Technically a valid construct, but against style. |
- args = Match(r'\s+(?:inline\s+)?%s\s*\(([^,()]+)\)' |
- % re.escape(base_classname), |
- line) |
- if (args and |
- args.group(1) != 'void' and |
- not Search(r'\bstd::initializer_list\b', args.group(1)) and |
- not Match(r'(const\s+)?%s(\s+const)?\s*(?:<\w+>\s*)?&' |
- % re.escape(base_classname), args.group(1).strip())): |
- error(filename, linenum, 'runtime/explicit', 5, |
- 'Single-argument constructors should be marked explicit.') |
+ # Technically a valid construct, but against style. Also look for |
+ # non-single-argument constructors which are also technically valid, but |
+ # strongly suggest something is wrong. |
+ explicit_constructor_match = Match( |
+ r'\s+(?:inline\s+)?(explicit\s+)?(?:inline\s+)?%s\s*' |
+ r'\(((?:[^()]|\([^()]*\))*)\)' |
+ % re.escape(base_classname), |
+ line) |
+ |
+ if explicit_constructor_match: |
+ is_marked_explicit = explicit_constructor_match.group(1) |
+ |
+ if not explicit_constructor_match.group(2): |
+ constructor_args = [] |
+ else: |
+ constructor_args = explicit_constructor_match.group(2).split(',') |
+ |
+ # collapse arguments so that commas in template parameter lists and function |
+ # argument parameter lists don't split arguments in two |
+ i = 0 |
+ while i < len(constructor_args): |
+ constructor_arg = constructor_args[i] |
+ while (constructor_arg.count('<') > constructor_arg.count('>') or |
+ constructor_arg.count('(') > constructor_arg.count(')')): |
+ constructor_arg += ',' + constructor_args[i + 1] |
+ del constructor_args[i + 1] |
+ constructor_args[i] = constructor_arg |
+ i += 1 |
+ |
+ defaulted_args = [arg for arg in constructor_args if '=' in arg] |
+ noarg_constructor = (not constructor_args or # empty arg list |
+ # 'void' arg specifier |
+ (len(constructor_args) == 1 and |
+ constructor_args[0].strip() == 'void')) |
+ onearg_constructor = ((len(constructor_args) == 1 and # exactly one arg |
+ not noarg_constructor) or |
+ # all but at most one arg defaulted |
+ (len(constructor_args) >= 1 and |
+ not noarg_constructor and |
+ len(defaulted_args) >= len(constructor_args) - 1)) |
+ initializer_list_constructor = bool( |
+ onearg_constructor and |
+ Search(r'\bstd\s*::\s*initializer_list\b', constructor_args[0])) |
+ copy_constructor = bool( |
+ onearg_constructor and |
+ Match(r'(const\s+)?%s(\s*<[^>]*>)?(\s+const)?\s*(?:<\w+>\s*)?&' |
+ % re.escape(base_classname), constructor_args[0].strip())) |
+ |
+ if (not is_marked_explicit and |
+ onearg_constructor and |
+ not initializer_list_constructor and |
+ not copy_constructor): |
+ if defaulted_args: |
+ error(filename, linenum, 'runtime/explicit', 5, |
+ 'Constructors callable with one argument ' |
+ 'should be marked explicit.') |
+ else: |
+ error(filename, linenum, 'runtime/explicit', 5, |
+ 'Single-parameter constructors should be marked explicit.') |
+ elif is_marked_explicit and not onearg_constructor: |
+ if noarg_constructor: |
+ error(filename, linenum, 'runtime/explicit', 5, |
+ 'Zero-parameter constructors should not be marked explicit.') |
+ else: |
+ error(filename, linenum, 'runtime/explicit', 0, |
+ 'Constructors that require multiple arguments ' |
+ 'should not be marked explicit.') |
def CheckSpacingForFunctionCall(filename, clean_lines, linenum, error): |
@@ -2634,6 +2756,20 @@ def IsBlankLine(line): |
return not line or line.isspace() |
+def CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, |
+ error): |
+ is_namespace_indent_item = ( |
+ len(nesting_state.stack) > 1 and |
+ nesting_state.stack[-1].check_namespace_indentation and |
+ isinstance(nesting_state.previous_stack_top, _NamespaceInfo) and |
+ nesting_state.previous_stack_top == nesting_state.stack[-2]) |
+ |
+ if ShouldCheckNamespaceIndentation(nesting_state, is_namespace_indent_item, |
+ clean_lines.elided, line): |
+ CheckItemIndentationInNamespace(filename, clean_lines.elided, |
+ line, error) |
+ |
+ |
def CheckForFunctionLengths(filename, clean_lines, linenum, |
function_state, error): |
"""Reports for long function bodies. |
@@ -2772,7 +2908,6 @@ def CheckAccess(filename, clean_lines, linenum, nesting_state, error): |
line = clean_lines.elided[linenum] # get rid of comments and strings |
matched = Match((r'\s*(DISALLOW_COPY_AND_ASSIGN|' |
- r'DISALLOW_EVIL_CONSTRUCTORS|' |
r'DISALLOW_IMPLICIT_CONSTRUCTORS)'), line) |
if not matched: |
return |
@@ -2994,6 +3129,7 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): |
# We allow no-spaces around << when used like this: 10<<20, but |
# not otherwise (particularly, not when used as streams) |
+ # |
# We also allow operators following an opening parenthesis, since |
# those tend to be macros that deal with operators. |
match = Search(r'(operator|\S)(?:L|UL|ULL|l|ul|ull)?<<([^\s,=])', line) |
@@ -3087,7 +3223,8 @@ def CheckCommaSpacing(filename, clean_lines, linenum, error): |
# verify that lines contain missing whitespaces, second pass on raw |
# lines to confirm that those missing whitespaces are not due to |
# elided comments. |
- if Search(r',[^,\s]', line) and Search(r',[^,\s]', raw[linenum]): |
+ if (Search(r',[^,\s]', ReplaceAll(r'\boperator\s*,\s*\(', 'F(', line)) and |
+ Search(r',[^,\s]', raw[linenum])): |
error(filename, linenum, 'whitespace/comma', 3, |
'Missing space after ,') |
@@ -3392,7 +3529,7 @@ def IsRValueType(clean_lines, nesting_state, linenum, column): |
match_symbol.group(1)) |
if match_func: |
# Check for constructors, which don't have return types. |
- if Search(r'\bexplicit$', match_func.group(1)): |
+ if Search(r'\b(?:explicit|inline)$', match_func.group(1)): |
return True |
implicit_constructor = Match(r'\s*(\w+)\((?:const\s+)?(\w+)', prefix) |
if (implicit_constructor and |
@@ -3417,8 +3554,27 @@ def IsRValueType(clean_lines, nesting_state, linenum, column): |
return False |
+def IsDeletedOrDefault(clean_lines, linenum): |
+ """Check if current constructor or operator is deleted or default. |
+ |
+ Args: |
+ clean_lines: A CleansedLines instance containing the file. |
+ linenum: The number of the line to check. |
+ Returns: |
+ True if this is a deleted or default constructor. |
+ """ |
+ open_paren = clean_lines.elided[linenum].find('(') |
+ if open_paren < 0: |
+ return False |
+ (close_line, _, close_paren) = CloseExpression( |
+ clean_lines, linenum, open_paren) |
+ if close_paren < 0: |
+ return False |
+ return Match(r'\s*=\s*(?:delete|default)\b', close_line[close_paren:]) |
+ |
+ |
def IsRValueAllowed(clean_lines, linenum): |
- """Check if RValue reference is allowed within some range of lines. |
+ """Check if RValue reference is allowed on a particular line. |
Args: |
clean_lines: A CleansedLines instance containing the file. |
@@ -3426,6 +3582,7 @@ def IsRValueAllowed(clean_lines, linenum): |
Returns: |
True if line is within the region where RValue references are allowed. |
""" |
+ # Allow region marked by PUSH/POP macros |
for i in xrange(linenum, 0, -1): |
line = clean_lines.elided[i] |
if Match(r'GOOGLE_ALLOW_RVALUE_REFERENCES_(?:PUSH|POP)', line): |
@@ -3435,6 +3592,26 @@ def IsRValueAllowed(clean_lines, linenum): |
line = clean_lines.elided[j] |
if Match(r'GOOGLE_ALLOW_RVALUE_REFERENCES_(?:PUSH|POP)', line): |
return line.endswith('POP') |
+ |
+ # Allow operator= |
+ line = clean_lines.elided[linenum] |
+ if Search(r'\boperator\s*=\s*\(', line): |
+ return IsDeletedOrDefault(clean_lines, linenum) |
+ |
+ # Allow constructors |
+ match = Match(r'\s*([\w<>]+)\s*::\s*([\w<>]+)\s*\(', line) |
+ if match and match.group(1) == match.group(2): |
+ return IsDeletedOrDefault(clean_lines, linenum) |
+ if Search(r'\b(?:explicit|inline)\s+[\w<>]+\s*\(', line): |
+ return IsDeletedOrDefault(clean_lines, linenum) |
+ |
+ if Match(r'\s*[\w<>]+\s*\(', line): |
+ previous_line = 'ReturnType' |
+ if linenum > 0: |
+ previous_line = clean_lines.elided[linenum - 1] |
+ if Match(r'^\s*$', previous_line) or Search(r'[{}:;]\s*$', previous_line): |
+ return IsDeletedOrDefault(clean_lines, linenum) |
+ |
return False |
@@ -3643,9 +3820,13 @@ def CheckBraces(filename, clean_lines, linenum, error): |
# methods) and a single \ after the semicolon (for macros) |
endpos = endline.find(';') |
if not Match(r';[\s}]*(\\?)$', endline[endpos:]): |
- # Semicolon isn't the last character, there's something trailing |
- error(filename, linenum, 'readability/braces', 4, |
- 'If/else bodies with multiple statements require braces') |
+ # Semicolon isn't the last character, there's something trailing. |
+ # Output a warning if the semicolon is not contained inside |
+ # a lambda expression. |
+ if not Match(r'^[^{};]*\[[^\[\]]*\][^{}]*\{[^{}]*\}\s*\)*[;,]\s*$', |
+ endline): |
+ error(filename, linenum, 'readability/braces', 4, |
+ 'If/else bodies with multiple statements require braces') |
elif endlinenum < len(clean_lines.elided) - 1: |
# Make sure the next line is dedented |
next_line = clean_lines.elided[endlinenum + 1] |
@@ -3876,6 +4057,13 @@ def CheckCheck(filename, clean_lines, linenum, error): |
clean_lines, linenum, start_pos) |
if end_pos < 0: |
return |
+ |
+ # If the check macro is followed by something other than a |
+ # semicolon, assume users will log their own custom error messages |
+ # and don't suggest any replacements. |
+ if not Match(r'\s*;', last_line[end_pos:]): |
+ return |
+ |
if linenum == end_line: |
expression = lines[linenum][start_pos + 1:end_pos - 1] |
else: |
@@ -4139,7 +4327,6 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, |
CheckSectionSpacing(filename, clean_lines, classinfo, linenum, error) |
-_RE_PATTERN_INCLUDE_NEW_STYLE = re.compile(r'#include +"[^/]+\.h"') |
_RE_PATTERN_INCLUDE = re.compile(r'^\s*#\s*include\s*([<"])([^>"]*)[>"].*$') |
# Matches the first component of a filename delimited by -s and _s. That is: |
# _RE_FIRST_COMPONENT.match('foo').group(0) == 'foo' |
@@ -4271,7 +4458,14 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): |
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(line): |
+ # Only do this check if the included header follows google naming |
+ # conventions. If not, assume that it's a 3rd party API that |
+ # requires special include conventions. |
+ # |
+ # We also make an exception for Lua headers, which follow google |
+ # naming convention but not the include convention. |
+ match = Match(r'#include\s*"([^/]+\.h)"', line) |
+ if match and not _THIRD_PARTY_HEADERS_PATTERN.match(match.group(1)): |
error(filename, linenum, 'build/include', 4, |
'Include the directory when naming .h files') |
@@ -4282,12 +4476,13 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): |
if match: |
include = match.group(2) |
is_system = (match.group(1) == '<') |
- if include in include_state: |
+ duplicate_line = include_state.FindHeader(include) |
+ if duplicate_line >= 0: |
error(filename, linenum, 'build/include', 4, |
'"%s" already included at %s:%s' % |
- (include, filename, include_state[include])) |
- else: |
- include_state[include] = linenum |
+ (include, filename, duplicate_line)) |
+ elif not _THIRD_PARTY_HEADERS_PATTERN.match(include): |
+ include_state.include_list[-1].append((include, linenum)) |
# We want to ensure that headers appear in the right order: |
# 1) for foo.cc, foo.h (preferred location) |
@@ -4441,8 +4636,9 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, |
# Reset include state across preprocessor directives. This is meant |
# to silence warnings for conditional includes. |
- if Match(r'^\s*#\s*(?:ifdef|elif|else|endif)\b', line): |
- include_state.ResetSection() |
+ match = Match(r'^\s*#\s*(if|ifdef|ifndef|elif|else|endif)\b', line) |
+ if match: |
+ include_state.ResetSection(match.group(1)) |
# Make Windows paths like Unix. |
fullname = os.path.abspath(filename).replace('\\', '/') |
@@ -4456,7 +4652,7 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, |
# TODO(unknown): check that 1-arg constructors are explicit. |
# How to tell it's a constructor? |
# (handled in CheckForNonStandardConstructs for now) |
- # TODO(unknown): check that classes have DISALLOW_EVIL_CONSTRUCTORS |
+ # TODO(unknown): check that classes declare or disable copy/assign |
# (level 1 error) |
pass |
@@ -4556,12 +4752,11 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, |
'Do not use variable-length arrays. Use an appropriately named ' |
"('k' followed by CamelCase) compile-time constant for the size.") |
- # If DISALLOW_EVIL_CONSTRUCTORS, DISALLOW_COPY_AND_ASSIGN, or |
- # DISALLOW_IMPLICIT_CONSTRUCTORS is present, then it should be the last thing |
- # in the class declaration. |
+ # If DISALLOW_COPY_AND_ASSIGN DISALLOW_IMPLICIT_CONSTRUCTORS is present, |
+ # then it should be the last thing in the class declaration. |
match = Match( |
(r'\s*' |
- r'(DISALLOW_(EVIL_CONSTRUCTORS|COPY_AND_ASSIGN|IMPLICIT_CONSTRUCTORS))' |
+ r'(DISALLOW_(COPY_AND_ASSIGN|IMPLICIT_CONSTRUCTORS))' |
r'\(.*\);$'), |
line) |
if match and linenum + 1 < clean_lines.NumLines(): |
@@ -4599,12 +4794,17 @@ def CheckGlobalStatic(filename, clean_lines, linenum, error): |
""" |
line = clean_lines.elided[linenum] |
+ # Match two lines at a time to support multiline declarations |
+ if linenum + 1 < clean_lines.NumLines() and not Search(r'[;({]', line): |
+ line += clean_lines.elided[linenum + 1].strip() |
+ |
# Check for people declaring static/global STL strings at the top level. |
# This is dangerous because the C++ language does not guarantee that |
# globals with constructors are initialized before the first access. |
match = Match( |
r'((?:|static +)(?:|const +))string +([a-zA-Z0-9_:]+)\b(.*)', |
line) |
+ |
# Remove false positives: |
# - String pointers (as opposed to values). |
# string *pointer |
@@ -4624,7 +4824,7 @@ def CheckGlobalStatic(filename, clean_lines, linenum, error): |
if (match and |
not Search(r'\bstring\b(\s+const)?\s*\*\s*(const\s+)?\w', line) and |
not Search(r'\boperator\W', line) and |
- not Match(r'\s*(<.*>)?(::[a-zA-Z0-9_]+)?\s*\(([^"]|$)', match.group(3))): |
+ not Match(r'\s*(<.*>)?(::[a-zA-Z0-9_]+)*\s*\(([^"]|$)', match.group(3))): |
error(filename, linenum, 'runtime/string', 4, |
'For a static/global string constant, use a C style string instead: ' |
'"%schar %s[]".' % |
@@ -4655,10 +4855,10 @@ def CheckPrintf(filename, clean_lines, linenum, error): |
'to snprintf.' % (match.group(1), match.group(2))) |
# Check if some verboten C functions are being used. |
- if Search(r'\bsprintf\b', line): |
+ if Search(r'\bsprintf\s*\(', line): |
error(filename, linenum, 'runtime/printf', 5, |
'Never use sprintf. Use snprintf instead.') |
- match = Search(r'\b(strcpy|strcat)\b', line) |
+ match = Search(r'\b(strcpy|strcat)\s*\(', line) |
if match: |
error(filename, linenum, 'runtime/printf', 4, |
'Almost always, snprintf is better than %s' % match.group(1)) |
@@ -4674,13 +4874,16 @@ def IsDerivedFunction(clean_lines, linenum): |
True if current line contains a function with "override" |
virt-specifier. |
""" |
- # Look for leftmost opening parenthesis on current line |
- opening_paren = clean_lines.elided[linenum].find('(') |
- if opening_paren < 0: return False |
- |
- # Look for "override" after the matching closing parenthesis |
- line, _, closing_paren = CloseExpression(clean_lines, linenum, opening_paren) |
- return closing_paren >= 0 and Search(r'\boverride\b', line[closing_paren:]) |
+ # Scan back a few lines for start of current function |
+ for i in xrange(linenum, max(-1, linenum - 10), -1): |
+ match = Match(r'^([^()]*\w+)\(', clean_lines.elided[i]) |
+ if match: |
+ # Look for "override" after the matching closing parenthesis |
+ line, _, closing_paren = CloseExpression( |
+ clean_lines, i, len(match.group(1))) |
+ return (closing_paren >= 0 and |
+ Search(r'\boverride\b', line[closing_paren:])) |
+ return False |
def IsInitializerList(clean_lines, linenum): |
@@ -4806,6 +5009,20 @@ def CheckForNonConstReference(filename, clean_lines, linenum, |
# Not at toplevel, not within a class, and not within a namespace |
return |
+ # Avoid initializer lists. We only need to scan back from the |
+ # current line for something that starts with ':'. |
+ # |
+ # We don't need to check the current line, since the '&' would |
+ # appear inside the second set of parentheses on the current line as |
+ # opposed to the first set. |
+ if linenum > 0: |
+ for i in xrange(linenum - 1, max(0, linenum - 10), -1): |
+ previous_line = clean_lines.elided[i] |
+ if not Search(r'[),]\s*$', previous_line): |
+ break |
+ if Match(r'^\s*:\s+\S', previous_line): |
+ return |
+ |
# Avoid preprocessors |
if Search(r'\\\s*$', line): |
return |
@@ -4881,6 +5098,11 @@ def CheckCasts(filename, clean_lines, linenum, error): |
# value < double(42) // bracket + space = true positive |
matched_new_or_template = match.group(1) |
+ # Avoid arrays by looking for brackets that come after the closing |
+ # parenthesis. |
+ if Match(r'\([^()]+\)\s*\[', match.group(3)): |
+ return |
+ |
# Other things to ignore: |
# - Function pointers |
# - Casts to pointer types |
@@ -4900,28 +5122,35 @@ def CheckCasts(filename, clean_lines, linenum, error): |
matched_type) |
if not expecting_function: |
- CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], |
- 'static_cast', |
+ CheckCStyleCast(filename, clean_lines, linenum, 'static_cast', |
r'\((int|float|double|bool|char|u?int(16|32|64))\)', error) |
# This doesn't catch all cases. Consider (const char * const)"hello". |
# |
# (char *) "foo" should always be a const_cast (reinterpret_cast won't |
# compile). |
- if CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], |
- 'const_cast', r'\((char\s?\*+\s?)\)\s*"', error): |
+ if CheckCStyleCast(filename, clean_lines, linenum, 'const_cast', |
+ r'\((char\s?\*+\s?)\)\s*"', error): |
pass |
else: |
# Check pointer casts for other than string constants |
- CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], |
- 'reinterpret_cast', r'\((\w+\s?\*+\s?)\)', error) |
+ CheckCStyleCast(filename, clean_lines, linenum, 'reinterpret_cast', |
+ r'\((\w+\s?\*+\s?)\)', error) |
# In addition, we look for people taking the address of a cast. This |
# is dangerous -- casts can assign to temporaries, so the pointer doesn't |
# point where you think. |
+ # |
+ # Some non-identifier character is required before the '&' for the |
+ # expression to be recognized as a cast. These are casts: |
+ # expression = &static_cast<int*>(temporary()); |
+ # function(&(int*)(temporary())); |
+ # |
+ # This is not a cast: |
+ # reference_type&(int* function_param); |
match = Search( |
- r'(?:&\(([^)]+)\)[\w(])|' |
- r'(?:&(static|dynamic|down|reinterpret)_cast\b)', line) |
+ r'(?:[^\w]&\(([^)]+)\)[\w(])|' |
+ r'(?:[^\w]&(static|dynamic|down|reinterpret)_cast\b)', line) |
if match and match.group(1) != '*': |
# Try a better error message when the & is bound to something |
# dereferenced by the casted pointer, as opposed to the casted |
@@ -4951,15 +5180,13 @@ def CheckCasts(filename, clean_lines, linenum, error): |
'Take the address before doing the cast, rather than after')) |
-def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, |
- error): |
+def CheckCStyleCast(filename, clean_lines, linenum, cast_type, pattern, error): |
"""Checks for a C-style cast by looking for the pattern. |
Args: |
filename: The name of the current file. |
+ clean_lines: A CleansedLines instance containing the file. |
linenum: The number of the line to check. |
- line: The line of code to check. |
- raw_line: The raw line of code to check, with comments. |
cast_type: The string for the C++ cast to recommend. This is either |
reinterpret_cast, static_cast, or const_cast, depending. |
pattern: The regular expression used to find C-style casts. |
@@ -4969,19 +5196,26 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, |
True if an error was emitted. |
False otherwise. |
""" |
+ line = clean_lines.elided[linenum] |
match = Search(pattern, line) |
if not match: |
return False |
- # Exclude lines with keywords that tend to look like casts, and also |
- # macros which are generally troublesome. |
- if Match(r'.*\b(?:sizeof|alignof|alignas|[A-Z_]+)\s*$', |
- line[0:match.start(1) - 1]): |
+ # Exclude lines with keywords that tend to look like casts |
+ context = line[0:match.start(1) - 1] |
+ if Match(r'.*\b(?:sizeof|alignof|alignas|[_A-Z][_A-Z0-9]*)\s*$', context): |
+ return False |
+ |
+ # Try expanding current context to see if we one level of |
+ # parentheses inside a macro. |
+ if linenum > 0: |
+ for i in xrange(linenum - 1, max(0, linenum - 5), -1): |
+ context = clean_lines.elided[i] + context |
+ if Match(r'.*\b[_A-Z][_A-Z0-9]*\s*\((?:\([^()]*\)|[^()])*$', context): |
return False |
# operator++(int) and operator--(int) |
- if (line[0:match.start(1) - 1].endswith(' operator++') or |
- line[0:match.start(1) - 1].endswith(' operator--')): |
+ if context.endswith(' operator++') or context.endswith(' operator--'): |
return False |
# A single unnamed argument for a function tends to look like old |
@@ -5005,10 +5239,11 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, |
# (FunctionPointer)(int); |
# (FunctionPointer)(int) = value; |
# Function((function_pointer_arg)(int)) |
+ # Function((function_pointer_arg)(int), int param) |
# <TemplateArgument(int)>; |
# <(FunctionPointerTemplateArgument)(int)>; |
remainder = line[match.end(0):] |
- if Match(r'^\s*(?:;|const\b|throw\b|final\b|override\b|=|>|\{|\))', |
+ if Match(r'^\s*(?:;|const\b|throw\b|final\b|override\b|[=>{),])', |
remainder): |
# Looks like an unnamed parameter. |
@@ -5031,6 +5266,7 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, |
# Don't warn if the parameter is named with block comments, e.g.: |
# Function(int /*unused_param*/); |
+ raw_line = clean_lines.raw_lines[linenum] |
if '/*' in raw_line: |
return False |
@@ -5182,16 +5418,16 @@ def FilesBelongToSameModule(filename_cc, 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. |
+def UpdateIncludeState(filename, include_dict, io=codecs): |
+ """Fill up the include_dict 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. |
+ include_dict: a dictionary 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. |
+ True if a header was successfully added. False otherwise. |
""" |
headerfile = None |
try: |
@@ -5205,9 +5441,7 @@ def UpdateIncludeState(filename, include_state, io=codecs): |
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)) |
+ include_dict.setdefault(include, linenum) |
return True |
@@ -5260,10 +5494,11 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, |
# 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() |
+ # Let's flatten the include_state include_list and copy it into a dictionary. |
+ include_dict = dict([item for sublist in include_state.include_list |
+ for item in sublist]) |
- # Did we find the header for this file (if any) and succesfully load it? |
+ # Did we find the header for this file (if any) and successfully load it? |
header_found = False |
# Use the absolute path so that matching works properly. |
@@ -5278,13 +5513,13 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, |
# instead of 'foo_flymake.h' |
abs_filename = re.sub(r'_flymake\.cc$', '.cc', abs_filename) |
- # include_state is modified during iteration, so we iterate over a copy of |
+ # include_dict is modified during iteration, so we iterate over a copy of |
# the keys. |
- header_keys = include_state.keys() |
+ header_keys = include_dict.keys() |
for header in header_keys: |
(same_module, common_path) = FilesBelongToSameModule(abs_filename, header) |
fullpath = common_path + header |
- if same_module and UpdateIncludeState(fullpath, include_state, io): |
+ if same_module and UpdateIncludeState(fullpath, include_dict, io): |
header_found = True |
# If we can't find the header file for a .cc, assume it's because we don't |
@@ -5298,7 +5533,7 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, |
# All the lines have been processed, report the errors found. |
for required_header_unstripped in required: |
template = required[required_header_unstripped][1] |
- if required_header_unstripped.strip('<>"') not in include_state: |
+ if required_header_unstripped.strip('<>"') not in include_dict: |
error(filename, required[required_header_unstripped][0], |
'build/include_what_you_use', 4, |
'Add #include ' + required_header_unstripped + ' for ' + template) |
@@ -5326,6 +5561,8 @@ def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error): |
4, # 4 = high confidence |
'For C++11-compatibility, omit template arguments from make_pair' |
' OR use pair directly OR if appropriate, construct a pair directly') |
+ |
+ |
def CheckDefaultLambdaCaptures(filename, clean_lines, linenum, error): |
"""Check that default lambda captures are not used. |
@@ -5351,6 +5588,139 @@ def CheckDefaultLambdaCaptures(filename, clean_lines, linenum, error): |
'Default lambda captures are an unapproved C++ feature.') |
+def CheckRedundantVirtual(filename, clean_lines, linenum, error): |
+ """Check if line contains a redundant "virtual" function-specifier. |
+ |
+ Args: |
+ filename: The name of the current file. |
+ clean_lines: A CleansedLines instance containing the file. |
+ linenum: The number of the line to check. |
+ error: The function to call with any errors found. |
+ """ |
+ # Look for "virtual" on current line. |
+ line = clean_lines.elided[linenum] |
+ virtual = Match(r'^(.*\bvirtual\b)', line) |
+ if not virtual: return |
+ |
+ # Look for the next opening parenthesis. This is the start of the |
+ # parameter list (possibly on the next line shortly after virtual). |
+ # TODO(unknown): doesn't work if there are virtual functions with |
+ # decltype() or other things that use parentheses, but csearch suggests |
+ # that this is rare. |
+ end_col = -1 |
+ end_line = -1 |
+ start_col = len(virtual.group(1)) |
+ for start_line in xrange(linenum, min(linenum + 3, clean_lines.NumLines())): |
+ line = clean_lines.elided[start_line][start_col:] |
+ parameter_list = Match(r'^([^(]*)\(', line) |
+ if parameter_list: |
+ # Match parentheses to find the end of the parameter list |
+ (_, end_line, end_col) = CloseExpression( |
+ clean_lines, start_line, start_col + len(parameter_list.group(1))) |
+ break |
+ start_col = 0 |
+ |
+ if end_col < 0: |
+ return # Couldn't find end of parameter list, give up |
+ |
+ # Look for "override" or "final" after the parameter list |
+ # (possibly on the next few lines). |
+ for i in xrange(end_line, min(end_line + 3, clean_lines.NumLines())): |
+ line = clean_lines.elided[i][end_col:] |
+ match = Search(r'\b(override|final)\b', line) |
+ if match: |
+ error(filename, linenum, 'readability/inheritance', 4, |
+ ('"virtual" is redundant since function is ' |
+ 'already declared as "%s"' % match.group(1))) |
+ |
+ # Set end_col to check whole lines after we are done with the |
+ # first line. |
+ end_col = 0 |
+ if Search(r'[^\w]\s*$', line): |
+ break |
+ |
+ |
+def CheckRedundantOverrideOrFinal(filename, clean_lines, linenum, error): |
+ """Check if line contains a redundant "override" or "final" virt-specifier. |
+ |
+ Args: |
+ filename: The name of the current file. |
+ clean_lines: A CleansedLines instance containing the file. |
+ linenum: The number of the line to check. |
+ error: The function to call with any errors found. |
+ """ |
+ # Check that at most one of "override" or "final" is present, not both |
+ line = clean_lines.elided[linenum] |
+ if Search(r'\boverride\b', line) and Search(r'\bfinal\b', line): |
+ error(filename, linenum, 'readability/inheritance', 4, |
+ ('"override" is redundant since function is ' |
+ 'already declared as "final"')) |
+ |
+ |
+ |
+ |
+# Returns true if we are at a new block, and it is directly |
+# inside of a namespace. |
+def IsBlockInNameSpace(nesting_state, is_forward_declaration): |
+ """Checks that the new block is directly in a namespace. |
+ |
+ Args: |
+ nesting_state: The _NestingState object that contains info about our state. |
+ is_forward_declaration: If the class is a forward declared class. |
+ Returns: |
+ Whether or not the new block is directly in a namespace. |
+ """ |
+ if is_forward_declaration: |
+ if len(nesting_state.stack) >= 1 and ( |
+ isinstance(nesting_state.stack[-1], _NamespaceInfo)): |
+ return True |
+ else: |
+ return False |
+ |
+ return (len(nesting_state.stack) > 1 and |
+ nesting_state.stack[-1].check_namespace_indentation and |
+ isinstance(nesting_state.stack[-2], _NamespaceInfo)) |
+ |
+ |
+def ShouldCheckNamespaceIndentation(nesting_state, is_namespace_indent_item, |
+ raw_lines_no_comments, linenum): |
+ """This method determines if we should apply our namespace indentation check. |
+ |
+ Args: |
+ nesting_state: The current nesting state. |
+ is_namespace_indent_item: If we just put a new class on the stack, True. |
+ If the top of the stack is not a class, or we did not recently |
+ add the class, False. |
+ raw_lines_no_comments: The lines without the comments. |
+ linenum: The current line number we are processing. |
+ |
+ Returns: |
+ True if we should apply our namespace indentation check. Currently, it |
+ only works for classes and namespaces inside of a namespace. |
+ """ |
+ |
+ is_forward_declaration = IsForwardClassDeclaration(raw_lines_no_comments, |
+ linenum) |
+ |
+ if not (is_namespace_indent_item or is_forward_declaration): |
+ return False |
+ |
+ # If we are in a macro, we do not want to check the namespace indentation. |
+ if IsMacroDefinition(raw_lines_no_comments, linenum): |
+ return False |
+ |
+ return IsBlockInNameSpace(nesting_state, is_forward_declaration) |
+ |
+ |
+# Call this method if the line is directly inside of a namespace. |
+# If the line above is blank (excluding comments) or the start of |
+# an inner namespace, it cannot be indented. |
+def CheckItemIndentationInNamespace(filename, raw_lines_no_comments, linenum, |
+ error): |
+ line = raw_lines_no_comments[linenum] |
+ if Match(r'^\s+', line): |
+ error(filename, linenum, 'runtime/indentation_namespace', 4, |
+ 'Do not indent within a namespace') |
def ProcessLine(filename, file_extension, clean_lines, line, |
@@ -5377,6 +5747,8 @@ def ProcessLine(filename, file_extension, clean_lines, line, |
raw_lines = clean_lines.raw_lines |
ParseNolintSuppressions(filename, raw_lines[line], line, error) |
nesting_state.Update(filename, clean_lines, line, error) |
+ CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, |
+ error) |
if nesting_state.InAsmBlock(): return |
CheckForFunctionLengths(filename, clean_lines, line, function_state, error) |
CheckForMultilineCommentsAndStrings(filename, clean_lines, line, error) |
@@ -5391,6 +5763,8 @@ def ProcessLine(filename, file_extension, clean_lines, line, |
CheckInvalidIncrement(filename, clean_lines, line, error) |
CheckMakePairUsesDeduction(filename, clean_lines, line, error) |
CheckDefaultLambdaCaptures(filename, clean_lines, line, error) |
+ CheckRedundantVirtual(filename, clean_lines, line, error) |
+ CheckRedundantOverrideOrFinal(filename, clean_lines, line, error) |
for check_fn in extra_check_functions: |
check_fn(filename, clean_lines, line, error) |