diff --git a/cpplint/cpplint.py b/cpplint/cpplint.py index 76d0735..36ad300 100755 --- a/cpplint/cpplint.py +++ b/cpplint/cpplint.py @@ -114,7 +114,7 @@ Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] ignored. Examples: - Assuing that src/.git exists, the header guard CPP variables for + Assuming that src/.git exists, the header guard CPP variables for src/chrome/browser/ui/browser.h are: No flag => CHROME_BROWSER_UI_BROWSER_H_ @@ -141,6 +141,7 @@ Syntax: cpplint.py [--verbose=#] [--output=vs7] [--filter=-x,+y,...] # here! cpplint_unittest.py should tell you if you forget to do this. _ERROR_CATEGORIES = [ 'build/class', + 'build/c++11', 'build/deprecated', 'build/endif_comment', 'build/explicit_make_pair', @@ -203,7 +204,7 @@ _ERROR_CATEGORIES = [ 'whitespace/todo' ] -# The default state of the category filter. This is overrided by the --filter= +# The default state of the category filter. This is overridden 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. @@ -213,7 +214,6 @@ _DEFAULT_FILTERS = ['-build/include_alpha'] # decided those were OK, as long as they were in UTF-8 and didn't represent # hard-coded international strings, which belong in a separate i18n file. - # C++ headers _CPP_HEADERS = frozenset([ # Legacy @@ -350,6 +350,7 @@ _CPP_HEADERS = frozenset([ 'cwctype', ]) + # 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. @@ -500,6 +501,7 @@ def IsErrorSuppressedByNolint(category, linenum): return (linenum in _error_suppressions.get(category, set()) or linenum in _error_suppressions.get(None, set())) + def Match(pattern, s): """Matches the string with the pattern, caching the compiled regexp.""" # The regexp compilation caching is inlined in both Match and Search for @@ -954,6 +956,7 @@ def _ShouldPrintError(category, confidence, linenum): # the verbosity level isn't high enough, or the filters filter it out. if IsErrorSuppressedByNolint(category, linenum): return False + if confidence < _cpplint_state.verbose_level: return False @@ -1011,11 +1014,9 @@ def Error(filename, linenum, category, confidence, message): # Matches standard C++ escape sequences per 2.13.2.3 of the C++ standard. _RE_PATTERN_CLEANSE_LINE_ESCAPES = re.compile( r'\\([abfnrtv?"\\\']|\d+|x[0-9a-fA-F]+)') -# Matches strings. Escape codes should already be removed by ESCAPES. -_RE_PATTERN_CLEANSE_LINE_DOUBLE_QUOTES = re.compile(r'"[^"]*"') -# Matches characters. Escape codes should already be removed by ESCAPES. -_RE_PATTERN_CLEANSE_LINE_SINGLE_QUOTES = re.compile(r"'.'") -# Matches multi-line C++ comments. +# Match a single C style comment on the same line. +_RE_PATTERN_C_COMMENTS = r'/\*(?:[^*]|\*(?!/))*\*/' +# Matches multi-line C style comments. # This RE is a little bit more complicated than one might expect, because we # have to take care of space removals tools so we can handle comments inside # statements better. @@ -1024,10 +1025,10 @@ _RE_PATTERN_CLEANSE_LINE_SINGLE_QUOTES = re.compile(r"'.'") # if this doesn't work we try on left side but only if there's a non-character # on the right. _RE_PATTERN_CLEANSE_LINE_C_COMMENTS = re.compile( - r"""(\s*/\*.*\*/\s*$| - /\*.*\*/\s+| - \s+/\*.*\*/(?=\W)| - /\*.*\*/)""", re.VERBOSE) + r'(\s*' + _RE_PATTERN_C_COMMENTS + r'\s*$|' + + _RE_PATTERN_C_COMMENTS + r'\s+|' + + r'\s+' + _RE_PATTERN_C_COMMENTS + r'(?=\W)|' + + _RE_PATTERN_C_COMMENTS + r')') def IsCppString(line): @@ -1082,9 +1083,12 @@ def CleanseRawStrings(raw_lines): delimiter = None else: # Haven't found the end yet, append a blank line. - line = '' + line = '""' - else: + # Look for beginning of a raw string, and replace them with + # empty strings. This is done in a loop to handle multiple raw + # strings on the same line. + while delimiter is None: # Look for beginning of a raw string. # See 2.14.15 [lex.string] for syntax. matched = Match(r'^(.*)\b(?:R|u8R|uR|UR|LR)"([^\s\\()]*)\((.*)$', line) @@ -1100,6 +1104,8 @@ def CleanseRawStrings(raw_lines): else: # Start of a multi-line raw string line = matched.group(1) + '""' + else: + break lines_without_raw_strings.append(line) @@ -1205,38 +1211,138 @@ class CleansedLines(object): Returns: The line with collapsed strings. """ - if not _RE_PATTERN_INCLUDE.match(elided): - # Remove escaped characters first to make quote/single quote collapsing - # basic. Things that look like escaped characters shouldn't occur - # outside of strings and chars. - elided = _RE_PATTERN_CLEANSE_LINE_ESCAPES.sub('', elided) - elided = _RE_PATTERN_CLEANSE_LINE_SINGLE_QUOTES.sub("''", elided) - elided = _RE_PATTERN_CLEANSE_LINE_DOUBLE_QUOTES.sub('""', elided) - return elided + if _RE_PATTERN_INCLUDE.match(elided): + return elided + + # Remove escaped characters first to make quote/single quote collapsing + # basic. Things that look like escaped characters shouldn't occur + # outside of strings and chars. + elided = _RE_PATTERN_CLEANSE_LINE_ESCAPES.sub('', elided) + + # Replace quoted strings and digit separators. Both single quotes + # and double quotes are processed in the same loop, otherwise + # nested quotes wouldn't work. + collapsed = '' + while True: + # Find the first quote character + match = Match(r'^([^\'"]*)([\'"])(.*)$', elided) + if not match: + collapsed += elided + break + head, quote, tail = match.groups() + + if quote == '"': + # Collapse double quoted strings + second_quote = tail.find('"') + if second_quote >= 0: + collapsed += head + '""' + elided = tail[second_quote + 1:] + else: + # Unmatched double quote, don't bother processing the rest + # of the line since this is probably a multiline string. + collapsed += elided + break + else: + # Found single quote, check nearby text to eliminate digit separators. + # + # There is no special handling for floating point here, because + # the integer/fractional/exponent parts would all be parsed + # correctly as long as there are digits on both sides of the + # separator. So we are fine as long as we don't see something + # like "0.'3" (gcc 4.9.0 will not allow this literal). + if Search(r'\b(?:0[bBxX]?|[1-9])[0-9a-fA-F]*$', head): + match_literal = Match(r'^((?:\'?[0-9a-zA-Z_])*)(.*)$', "'" + tail) + collapsed += head + match_literal.group(1).replace("'", '') + elided = match_literal.group(2) + else: + second_quote = tail.find('\'') + if second_quote >= 0: + collapsed += head + "''" + elided = tail[second_quote + 1:] + else: + # Unmatched single quote + collapsed += elided + break + + return collapsed -def FindEndOfExpressionInLine(line, startpos, depth, startchar, endchar): - """Find the position just after the matching endchar. +def FindEndOfExpressionInLine(line, startpos, stack): + """Find the position just after the end of current parenthesized expression. Args: line: a CleansedLines line. startpos: start searching at this position. - depth: nesting level at startpos. - startchar: expression opening character. - endchar: expression closing character. + stack: nesting stack at startpos. Returns: - On finding matching endchar: (index just after matching endchar, 0) - Otherwise: (-1, new depth at end of this line) + On finding matching end: (index just after matching end, None) + On finding an unclosed expression: (-1, None) + Otherwise: (-1, new stack at end of this line) """ for i in xrange(startpos, len(line)): - if line[i] == startchar: - depth += 1 - elif line[i] == endchar: - depth -= 1 - if depth == 0: - return (i + 1, 0) - return (-1, depth) + char = line[i] + if char in '([{': + # Found start of parenthesized expression, push to expression stack + stack.append(char) + elif char == '<': + # Found potential start of template argument list + if i > 0 and line[i - 1] == '<': + # Left shift operator + if stack and stack[-1] == '<': + stack.pop() + if not stack: + return (-1, None) + elif i > 0 and Search(r'\boperator\s*$', line[0:i]): + # operator<, don't add to stack + continue + else: + # Tentative start of template argument list + stack.append('<') + elif char in ')]}': + # Found end of parenthesized expression. + # + # If we are currently expecting a matching '>', the pending '<' + # must have been an operator. Remove them from expression stack. + while stack and stack[-1] == '<': + stack.pop() + if not stack: + return (-1, None) + if ((stack[-1] == '(' and char == ')') or + (stack[-1] == '[' and char == ']') or + (stack[-1] == '{' and char == '}')): + stack.pop() + if not stack: + return (i + 1, None) + else: + # Mismatched parentheses + return (-1, None) + elif char == '>': + # Found potential end of template argument list. + + # Ignore "->" and operator functions + if (i > 0 and + (line[i - 1] == '-' or Search(r'\boperator\s*$', line[0:i - 1]))): + continue + + # Pop the stack if there is a matching '<'. Otherwise, ignore + # this '>' since it must be an operator. + if stack: + if stack[-1] == '<': + stack.pop() + if not stack: + return (i + 1, None) + elif char == ';': + # Found something that look like end of statements. If we are currently + # expecting a '>', the matching '<' must have been an operator, since + # template argument list should not contain statements. + while stack and stack[-1] == '<': + stack.pop() + if not stack: + return (-1, None) + + # Did not find end of expression or unbalanced parentheses on this line + return (-1, stack) def CloseExpression(clean_lines, linenum, pos): @@ -1245,6 +1351,11 @@ def CloseExpression(clean_lines, linenum, pos): If lines[linenum][pos] points to a '(' or '{' or '[' or '<', finds the linenum/pos that correspond to the closing of the expression. + TODO(unknown): cpplint spends a fair bit of time matching parentheses. + Ideally we would want to index all opening and closing parentheses once + and have CloseExpression be just a simple lookup, but due to preprocessor + tricks, this is not so easy. + Args: clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. @@ -1258,35 +1369,28 @@ def CloseExpression(clean_lines, linenum, pos): """ line = clean_lines.elided[linenum] - startchar = line[pos] - if startchar not in '({[<': + if (line[pos] not in '({[<') or Match(r'<[<=]', line[pos:]): return (line, clean_lines.NumLines(), -1) - if startchar == '(': endchar = ')' - if startchar == '[': endchar = ']' - if startchar == '{': endchar = '}' - if startchar == '<': endchar = '>' # Check first line - (end_pos, num_open) = FindEndOfExpressionInLine( - line, pos, 0, startchar, endchar) + (end_pos, stack) = FindEndOfExpressionInLine(line, pos, []) if end_pos > -1: return (line, linenum, end_pos) # Continue scanning forward - while linenum < clean_lines.NumLines() - 1: + while stack and linenum < clean_lines.NumLines() - 1: linenum += 1 line = clean_lines.elided[linenum] - (end_pos, num_open) = FindEndOfExpressionInLine( - line, 0, num_open, startchar, endchar) + (end_pos, stack) = FindEndOfExpressionInLine(line, 0, stack) if end_pos > -1: return (line, linenum, end_pos) - # Did not find endchar before end of file, give up + # Did not find end of expression before end of file, give up return (line, clean_lines.NumLines(), -1) -def FindStartOfExpressionInLine(line, endpos, depth, startchar, endchar): - """Find position at the matching startchar. +def FindStartOfExpressionInLine(line, endpos, stack): + """Find position at the matching start of current expression. This is almost the reverse of FindEndOfExpressionInLine, but note that the input position and returned position differs by 1. @@ -1294,22 +1398,72 @@ def FindStartOfExpressionInLine(line, endpos, depth, startchar, endchar): Args: line: a CleansedLines line. endpos: start searching at this position. - depth: nesting level at endpos. - startchar: expression opening character. - endchar: expression closing character. + stack: nesting stack at endpos. Returns: - On finding matching startchar: (index at matching startchar, 0) - Otherwise: (-1, new depth at beginning of this line) + On finding matching start: (index at matching start, None) + On finding an unclosed expression: (-1, None) + Otherwise: (-1, new stack at beginning of this line) """ - for i in xrange(endpos, -1, -1): - if line[i] == endchar: - depth += 1 - elif line[i] == startchar: - depth -= 1 - if depth == 0: - return (i, 0) - return (-1, depth) + i = endpos + while i >= 0: + char = line[i] + if char in ')]}': + # Found end of expression, push to expression stack + stack.append(char) + elif char == '>': + # Found potential end of template argument list. + # + # Ignore it if it's a "->" or ">=" or "operator>" + if (i > 0 and + (line[i - 1] == '-' or + Match(r'\s>=\s', line[i - 1:]) or + Search(r'\boperator\s*$', line[0:i]))): + i -= 1 + else: + stack.append('>') + elif char == '<': + # Found potential start of template argument list + if i > 0 and line[i - 1] == '<': + # Left shift operator + i -= 1 + else: + # If there is a matching '>', we can pop the expression stack. + # Otherwise, ignore this '<' since it must be an operator. + if stack and stack[-1] == '>': + stack.pop() + if not stack: + return (i, None) + elif char in '([{': + # Found start of expression. + # + # If there are any unmatched '>' on the stack, they must be + # operators. Remove those. + while stack and stack[-1] == '>': + stack.pop() + if not stack: + return (-1, None) + if ((char == '(' and stack[-1] == ')') or + (char == '[' and stack[-1] == ']') or + (char == '{' and stack[-1] == '}')): + stack.pop() + if not stack: + return (i, None) + else: + # Mismatched parentheses + return (-1, None) + elif char == ';': + # Found something that look like end of statements. If we are currently + # expecting a '<', the matching '>' must have been an operator, since + # template argument list should not contain statements. + while stack and stack[-1] == '>': + stack.pop() + if not stack: + return (-1, None) + + i -= 1 + + return (-1, stack) def ReverseCloseExpression(clean_lines, linenum, pos): @@ -1330,30 +1484,23 @@ def ReverseCloseExpression(clean_lines, linenum, pos): return is the 'cleansed' line at linenum. """ line = clean_lines.elided[linenum] - endchar = line[pos] - if endchar not in ')}]>': + if line[pos] not in ')}]>': return (line, 0, -1) - if endchar == ')': startchar = '(' - if endchar == ']': startchar = '[' - if endchar == '}': startchar = '{' - if endchar == '>': startchar = '<' # Check last line - (start_pos, num_open) = FindStartOfExpressionInLine( - line, pos, 0, startchar, endchar) + (start_pos, stack) = FindStartOfExpressionInLine(line, pos, []) if start_pos > -1: return (line, linenum, start_pos) # Continue scanning backward - while linenum > 0: + while stack and linenum > 0: linenum -= 1 line = clean_lines.elided[linenum] - (start_pos, num_open) = FindStartOfExpressionInLine( - line, len(line) - 1, num_open, startchar, endchar) + (start_pos, stack) = FindStartOfExpressionInLine(line, len(line) - 1, stack) if start_pos > -1: return (line, linenum, start_pos) - # Did not find startchar before beginning of file, give up + # Did not find start of expression before beginning of file, give up return (line, 0, -1) @@ -1370,6 +1517,22 @@ def CheckForCopyright(filename, lines, error): 'You should have a line: "Copyright [year] "') +def GetIndentLevel(line): + """Return the number of leading spaces in line. + + Args: + line: A string to check. + + Returns: + An integer count of leading spaces, possibly zero. + """ + indent = Match(r'^( *)\S', line) + if indent: + return len(indent.group(1)) + else: + return 0 + + def GetHeaderGuardCPPVariable(filename): """Returns the CPP variable that should be used as a header guard. @@ -1550,19 +1713,33 @@ def CheckForMultilineCommentsAndStrings(filename, clean_lines, linenum, error): 'Use C++11 raw strings or concatenation instead.') -threading_list = ( - ('asctime(', 'asctime_r('), - ('ctime(', 'ctime_r('), - ('getgrgid(', 'getgrgid_r('), - ('getgrnam(', 'getgrnam_r('), - ('getlogin(', 'getlogin_r('), - ('getpwnam(', 'getpwnam_r('), - ('getpwuid(', 'getpwuid_r('), - ('gmtime(', 'gmtime_r('), - ('localtime(', 'localtime_r('), - ('rand(', 'rand_r('), - ('strtok(', 'strtok_r('), - ('ttyname(', 'ttyname_r('), +# (non-threadsafe name, thread-safe alternative, validation pattern) +# +# The validation pattern is used to eliminate false positives such as: +# _rand(); // false positive due to substring match. +# ->rand(); // some member function rand(). +# ACMRandom rand(seed); // some variable named rand. +# ISAACRandom rand(); // another variable named rand. +# +# Basically we require the return value of these functions to be used +# in some expression context on the same line by matching on some +# operator before the function name. This eliminates constructors and +# member function calls. +_UNSAFE_FUNC_PREFIX = r'(?:[-+*/=%^&|(<]\s*|>\s+)' +_THREADING_LIST = ( + ('asctime(', 'asctime_r(', _UNSAFE_FUNC_PREFIX + r'asctime\([^)]+\)'), + ('ctime(', 'ctime_r(', _UNSAFE_FUNC_PREFIX + r'ctime\([^)]+\)'), + ('getgrgid(', 'getgrgid_r(', _UNSAFE_FUNC_PREFIX + r'getgrgid\([^)]+\)'), + ('getgrnam(', 'getgrnam_r(', _UNSAFE_FUNC_PREFIX + r'getgrnam\([^)]+\)'), + ('getlogin(', 'getlogin_r(', _UNSAFE_FUNC_PREFIX + r'getlogin\(\)'), + ('getpwnam(', 'getpwnam_r(', _UNSAFE_FUNC_PREFIX + r'getpwnam\([^)]+\)'), + ('getpwuid(', 'getpwuid_r(', _UNSAFE_FUNC_PREFIX + r'getpwuid\([^)]+\)'), + ('gmtime(', 'gmtime_r(', _UNSAFE_FUNC_PREFIX + r'gmtime\([^)]+\)'), + ('localtime(', 'localtime_r(', _UNSAFE_FUNC_PREFIX + r'localtime\([^)]+\)'), + ('rand(', 'rand_r(', _UNSAFE_FUNC_PREFIX + r'rand\(\)'), + ('strtok(', 'strtok_r(', + _UNSAFE_FUNC_PREFIX + r'strtok\([^)]+\)'), + ('ttyname(', 'ttyname_r(', _UNSAFE_FUNC_PREFIX + r'ttyname\([^)]+\)'), ) @@ -1582,14 +1759,13 @@ def CheckPosixThreading(filename, clean_lines, linenum, error): error: The function to call with any errors found. """ line = clean_lines.elided[linenum] - for single_thread_function, multithread_safe_function in threading_list: - ix = line.find(single_thread_function) - # Comparisons made explicit for clarity -- pylint: disable=g-explicit-bool-comparison - if ix >= 0 and (ix == 0 or (not line[ix - 1].isalnum() and - line[ix - 1] not in ('_', '.', '>'))): + for single_thread_func, multithread_safe_func, pattern in _THREADING_LIST: + # Additional pattern matching check to confirm that this is the + # function we are looking for + if Search(pattern, line): error(filename, linenum, 'runtime/threadsafe_fn', 2, - 'Consider using ' + multithread_safe_function + - '...) instead of ' + single_thread_function + + 'Consider using ' + multithread_safe_func + + '...) instead of ' + single_thread_func + '...) for improved thread safety.') @@ -1611,7 +1787,6 @@ def CheckVlogArguments(filename, clean_lines, linenum, error): 'VLOG() should be used with numeric verbosity level. ' 'Use LOG() if you want symbolic severity levels.') - # Matches invalid increment: *count++, which moves pointer instead of # incrementing a value. _RE_PATTERN_INVALID_INCREMENT = re.compile( @@ -1676,6 +1851,24 @@ class _BlockInfo(object): """ pass + def IsBlockInfo(self): + """Returns true if this block is a _BlockInfo. + + This is convenient for verifying that an object is an instance of + a _BlockInfo, but not an instance of any of the derived classes. + + Returns: + True for this class, False for derived classes. + """ + return self.__class__ == _BlockInfo + + +class _ExternCInfo(_BlockInfo): + """Stores information about an 'extern "C"' block.""" + + def __init__(self): + _BlockInfo.__init__(self, True) + class _ClassInfo(_BlockInfo): """Stores information about a class.""" @@ -1694,11 +1887,7 @@ class _ClassInfo(_BlockInfo): # Remember initial indentation level for this class. Using raw_lines here # instead of elided to account for leading comments. - initial_indent = Match(r'^( *)\S', clean_lines.raw_lines[linenum]) - if initial_indent: - self.class_indent = len(initial_indent.group(1)) - else: - self.class_indent = 0 + self.class_indent = GetIndentLevel(clean_lines.raw_lines[linenum]) # Try to find the end of the class. This will be confused by things like: # class A { @@ -1783,8 +1972,15 @@ class _NamespaceInfo(_BlockInfo): else: # Anonymous namespace if not Match(r'};*\s*(//|/\*).*\bnamespace[\*/\.\\\s]*$', line): - error(filename, linenum, 'readability/namespace', 5, - 'Namespace should be terminated with "// namespace"') + # If "// namespace anonymous" or "// anonymous namespace (more text)", + # mention "// anonymous namespace" as an acceptable form + if Match(r'}.*\b(namespace anonymous|anonymous namespace)\b', line): + error(filename, linenum, 'readability/namespace', 5, + 'Anonymous namespace should be terminated with "// namespace"' + ' or "// anonymous namespace"') + else: + error(filename, linenum, 'readability/namespace', 5, + 'Anonymous namespace should be terminated with "// namespace"') class _PreprocessorInfo(object): @@ -1801,7 +1997,7 @@ class _PreprocessorInfo(object): self.seen_else = False -class _NestingState(object): +class NestingState(object): """Holds states related to parsing braces.""" def __init__(self): @@ -1813,6 +2009,17 @@ class _NestingState(object): # - _BlockInfo: some other type of block. self.stack = [] + # Top of the previous stack before each Update(). + # + # Because the nesting_stack is updated at the end of each line, we + # had to do some convoluted checks to find out what is the current + # scope at the beginning of the line. This check is simplified by + # saving the previous top of nesting stack. + # + # We could save the full stack, but we only need the top. Copying + # the full nesting stack would slow down cpplint by ~10%. + self.previous_stack_top = [] + # Stack of _PreprocessorInfo objects. self.pp_stack = [] @@ -1833,6 +2040,82 @@ class _NestingState(object): """ return self.stack and isinstance(self.stack[-1], _NamespaceInfo) + def InExternC(self): + """Check if we are currently one level inside an 'extern "C"' block. + + Returns: + True if top of the stack is an extern block, False otherwise. + """ + return self.stack and isinstance(self.stack[-1], _ExternCInfo) + + def InClassDeclaration(self): + """Check if we are currently one level inside a class or struct declaration. + + Returns: + True if top of the stack is a class/struct, False otherwise. + """ + return self.stack and isinstance(self.stack[-1], _ClassInfo) + + def InAsmBlock(self): + """Check if we are currently one level inside an inline ASM block. + + Returns: + True if the top of the stack is a block containing inline ASM. + """ + return self.stack and self.stack[-1].inline_asm != _NO_ASM + + def InTemplateArgumentList(self, clean_lines, linenum, pos): + """Check if current position is inside template argument list. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + pos: position just after the suspected template argument. + Returns: + True if (linenum, pos) is inside template arguments. + """ + while linenum < clean_lines.NumLines(): + # Find the earliest character that might indicate a template argument + line = clean_lines.elided[linenum] + match = Match(r'^[^{};=\[\]\.<>]*(.)', line[pos:]) + if not match: + linenum += 1 + pos = 0 + continue + token = match.group(1) + pos += len(match.group(0)) + + # These things do not look like template argument list: + # class Suspect { + # class Suspect x; } + if token in ('{', '}', ';'): return False + + # These things look like template argument list: + # template + # template + # template + # template + if token in ('>', '=', '[', ']', '.'): return True + + # Check if token is an unmatched '<'. + # If not, move on to the next character. + if token != '<': + pos += 1 + if pos >= len(line): + linenum += 1 + pos = 0 + continue + + # We can't be sure if we just find a single '<', and need to + # find the matching '>'. + (_, end_line, end_pos) = CloseExpression(clean_lines, linenum, pos - 1) + if end_pos < 0: + # Not sure if template argument list or syntax error in file + return False + linenum = end_line + pos = end_pos + return False + def UpdatePreprocessor(self, line): """Update preprocessor stack. @@ -1889,6 +2172,7 @@ class _NestingState(object): # TODO(unknown): unexpected #endif, issue warning? pass + # TODO(unknown): Update() is too long, but we will refactor later. def Update(self, filename, clean_lines, linenum, error): """Update nesting state with current line. @@ -1900,7 +2184,17 @@ class _NestingState(object): """ line = clean_lines.elided[linenum] - # Update pp_stack first + # Remember top of the previous nesting stack. + # + # The stack is always pushed/popped and not modified in place, so + # we can just do a shallow copy instead of copy.deepcopy. Using + # deepcopy would slow down cpplint by ~28%. + if self.stack: + self.previous_stack_top = self.stack[-1] + else: + self.previous_stack_top = None + + # Update pp_stack self.UpdatePreprocessor(line) # Count parentheses. This is to avoid adding struct arguments to @@ -1951,32 +2245,27 @@ class _NestingState(object): # such as in: # class LOCKABLE API Object { # }; - # - # Templates with class arguments may confuse the parser, for example: - # template , - # class Vector = vector > - # class HeapQueue { - # - # Because this parser has no nesting state about templates, by the - # time it saw "class Comparator", it may think that it's a new class. - # Nested templates have a similar problem: - # template < - # typename ExportedType, - # typename TupleType, - # template class ImplTemplate> - # - # To avoid these cases, we ignore classes that are followed by '=' or '>' class_decl_match = Match( - r'\s*(template\s*<[\w\s<>,:]*>\s*)?' - r'(class|struct)\s+([A-Z_]+\s+)*(\w+(?:::\w+)*)' - r'(([^=>]|<[^<>]*>|<[^<>]*<[^<>]*>\s*>)*)$', line) + r'^(\s*(?:template\s*<[\w\s<>,:]*>\s*)?' + r'(class|struct)\s+(?:[A-Z_]+\s+)*(\w+(?:::\w+)*))' + r'(.*)$', line) if (class_decl_match and (not self.stack or self.stack[-1].open_parentheses == 0)): - self.stack.append(_ClassInfo( - class_decl_match.group(4), class_decl_match.group(2), - clean_lines, linenum)) - line = class_decl_match.group(5) + # We do not want to accept classes that are actually template arguments: + # template , + # template class Ignore3> + # void Function() {}; + # + # To avoid template argument cases, we scan forward and look for + # an unmatched '>'. If we see one, assume we are inside a + # template argument list. + end_declaration = len(class_decl_match.group(1)) + if not self.InTemplateArgumentList(clean_lines, linenum, end_declaration): + self.stack.append(_ClassInfo( + class_decl_match.group(3), class_decl_match.group(2), + clean_lines, linenum)) + line = class_decl_match.group(4) # If we have not yet seen the opening brace for the innermost block, # run checks here. @@ -2023,10 +2312,13 @@ class _NestingState(object): # stack otherwise. if not self.SeenOpenBrace(): self.stack[-1].seen_open_brace = True + elif Match(r'^extern\s*"[^"]*"\s*\{', line): + self.stack.append(_ExternCInfo()) else: self.stack.append(_BlockInfo(True)) if _MATCH_ASM.match(line): self.stack[-1].inline_asm = _BLOCK_ASM + elif token == ';' or token == ')': # If we haven't seen an opening brace yet, but we already saw # a semicolon, this is probably a forward declaration. Pop @@ -2102,7 +2394,7 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: A callable to which errors are reported, which takes 4 arguments: filename, line number, error level, and message @@ -2180,21 +2472,23 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, 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.') -def CheckSpacingForFunctionCall(filename, line, linenum, error): +def CheckSpacingForFunctionCall(filename, clean_lines, linenum, error): """Checks for the correctness of various spacing around function calls. Args: filename: The name of the current file. - line: The text of the line to check. + 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. """ + line = clean_lines.elided[linenum] # Since function calls often occur inside if/for/while/switch # expressions - which have their own, more liberal conventions - we @@ -2237,10 +2531,16 @@ def CheckSpacingForFunctionCall(filename, line, linenum, error): error(filename, linenum, 'whitespace/parens', 2, 'Extra space after (') if (Search(r'\w\s+\(', fncall) and - not Search(r'#\s*define|typedef', fncall) and + not Search(r'#\s*define|typedef|using\s+\w+\s*=', fncall) and not Search(r'\w\s+\((\w+::)*\*\w+\)\(', fncall)): - error(filename, linenum, 'whitespace/parens', 4, - 'Extra space before ( in function call') + # TODO(unknown): Space after an operator function seem to be a common + # error, silence those for now by restricting them to highest verbosity. + if Search(r'\boperator_*\b', line): + error(filename, linenum, 'whitespace/parens', 0, + 'Extra space before ( in function call') + else: + error(filename, linenum, 'whitespace/parens', 4, + 'Extra space before ( in function call') # If the ) is followed only by a newline or a { + newline, assume it's # part of a control statement (if/while/etc), and don't complain if Search(r'[^)]\s+\)\s*[^{\s]', fncall): @@ -2294,8 +2594,6 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, """ lines = clean_lines.lines line = lines[linenum] - raw = clean_lines.raw_lines - raw_line = raw[linenum] joined_line = '' starting_func = False @@ -2342,34 +2640,58 @@ def CheckForFunctionLengths(filename, clean_lines, linenum, _RE_PATTERN_TODO = re.compile(r'^//(\s*)TODO(\(.+?\))?:?(\s|$)?') -def CheckComment(comment, filename, linenum, error): - """Checks for common mistakes in TODO comments. +def CheckComment(line, filename, linenum, next_line_start, error): + """Checks for common mistakes in comments. Args: - comment: The text of the comment from the line in question. + line: The line in question. filename: The name of the current file. linenum: The number of the line to check. + next_line_start: The first non-whitespace column of the next line. error: The function to call with any errors found. """ - match = _RE_PATTERN_TODO.match(comment) - if match: - # One whitespace is correct; zero whitespace is handled elsewhere. - leading_whitespace = match.group(1) - if len(leading_whitespace) > 1: - error(filename, linenum, 'whitespace/todo', 2, - 'Too many spaces before TODO') + commentpos = line.find('//') + if commentpos != -1: + # Check if the // may be in quotes. If so, ignore it + # Comparisons made explicit for clarity -- pylint: disable=g-explicit-bool-comparison + if (line.count('"', 0, commentpos) - + line.count('\\"', 0, commentpos)) % 2 == 0: # not in quotes + # Allow one space for new scopes, two spaces otherwise: + if (not (Match(r'^.*{ *//', line) and next_line_start == commentpos) and + ((commentpos >= 1 and + line[commentpos-1] not in string.whitespace) or + (commentpos >= 2 and + line[commentpos-2] not in string.whitespace))): + error(filename, linenum, 'whitespace/comments', 2, + 'At least two spaces is best between code and comments') - username = match.group(2) - if not username: - error(filename, linenum, 'readability/todo', 2, - 'Missing username in TODO; it should look like ' - '"// TODO(my_username): Stuff."') + # Checks for common mistakes in TODO comments. + comment = line[commentpos:] + match = _RE_PATTERN_TODO.match(comment) + if match: + # One whitespace is correct; zero whitespace is handled elsewhere. + leading_whitespace = match.group(1) + if len(leading_whitespace) > 1: + error(filename, linenum, 'whitespace/todo', 2, + 'Too many spaces before TODO') - middle_whitespace = match.group(3) - # Comparisons made explicit for correctness -- pylint: disable=g-explicit-bool-comparison - if middle_whitespace != ' ' and middle_whitespace != '': - error(filename, linenum, 'whitespace/todo', 2, - 'TODO(my_username) should be followed by a space') + username = match.group(2) + if not username: + error(filename, linenum, 'readability/todo', 2, + 'Missing username in TODO; it should look like ' + '"// TODO(my_username): Stuff."') + + middle_whitespace = match.group(3) + # Comparisons made explicit for correctness -- pylint: disable=g-explicit-bool-comparison + if middle_whitespace != ' ' and middle_whitespace != '': + error(filename, linenum, 'whitespace/todo', 2, + 'TODO(my_username) should be followed by a space') + + # If the comment contains an alphanumeric character, there + # should be a space somewhere between it and the //. + if Match(r'//[^ ]*\w', comment): + error(filename, linenum, 'whitespace/comments', 4, + 'Should have a space between // and comment') def CheckAccess(filename, clean_lines, linenum, nesting_state, error): """Checks for improper use of DISALLOW* macros. @@ -2378,7 +2700,7 @@ def CheckAccess(filename, clean_lines, linenum, nesting_state, error): filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: The function to call with any errors found. """ @@ -2402,132 +2724,6 @@ def CheckAccess(filename, clean_lines, linenum, nesting_state, error): pass -def FindNextMatchingAngleBracket(clean_lines, linenum, init_suffix): - """Find the corresponding > to close a template. - - Args: - clean_lines: A CleansedLines instance containing the file. - linenum: Current line number. - init_suffix: Remainder of the current line after the initial <. - - Returns: - True if a matching bracket exists. - """ - line = init_suffix - nesting_stack = ['<'] - while True: - # Find the next operator that can tell us whether < is used as an - # opening bracket or as a less-than operator. We only want to - # warn on the latter case. - # - # We could also check all other operators and terminate the search - # early, e.g. if we got something like this "a(),;\[\]]*([<>(),;\[\]])(.*)$', line) - if match: - # Found an operator, update nesting stack - operator = match.group(1) - line = match.group(2) - - if nesting_stack[-1] == '<': - # Expecting closing angle bracket - if operator in ('<', '(', '['): - nesting_stack.append(operator) - elif operator == '>': - nesting_stack.pop() - if not nesting_stack: - # Found matching angle bracket - return True - elif operator == ',': - # Got a comma after a bracket, this is most likely a template - # argument. We have not seen a closing angle bracket yet, but - # it's probably a few lines later if we look for it, so just - # return early here. - return True - else: - # Got some other operator. - return False - - else: - # Expecting closing parenthesis or closing bracket - if operator in ('<', '(', '['): - nesting_stack.append(operator) - elif operator in (')', ']'): - # We don't bother checking for matching () or []. If we got - # something like (] or [), it would have been a syntax error. - nesting_stack.pop() - - else: - # Scan the next line - linenum += 1 - if linenum >= len(clean_lines.elided): - break - line = clean_lines.elided[linenum] - - # Exhausted all remaining lines and still no matching angle bracket. - # Most likely the input was incomplete, otherwise we should have - # seen a semicolon and returned early. - return True - - -def FindPreviousMatchingAngleBracket(clean_lines, linenum, init_prefix): - """Find the corresponding < that started a template. - - Args: - clean_lines: A CleansedLines instance containing the file. - linenum: Current line number. - init_prefix: Part of the current line before the initial >. - - Returns: - True if a matching bracket exists. - """ - line = init_prefix - nesting_stack = ['>'] - while True: - # Find the previous operator - match = Search(r'^(.*)([<>(),;\[\]])[^<>(),;\[\]]*$', line) - if match: - # Found an operator, update nesting stack - operator = match.group(2) - line = match.group(1) - - if nesting_stack[-1] == '>': - # Expecting opening angle bracket - if operator in ('>', ')', ']'): - nesting_stack.append(operator) - elif operator == '<': - nesting_stack.pop() - if not nesting_stack: - # Found matching angle bracket - return True - elif operator == ',': - # Got a comma before a bracket, this is most likely a - # template argument. The opening angle bracket is probably - # there if we look for it, so just return early here. - return True - else: - # Got some other operator. - return False - - else: - # Expecting opening parenthesis or opening bracket - if operator in ('>', ')', ']'): - nesting_stack.append(operator) - elif operator in ('(', '['): - nesting_stack.pop() - - else: - # Scan the previous line - linenum -= 1 - if linenum < 0: - break - line = clean_lines.elided[linenum] - - # Exhausted all earlier lines and still no matching angle bracket. - return False - - def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): """Checks for the correctness of various spacing issues in the code. @@ -2541,7 +2737,7 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: The function to call with any errors found. """ @@ -2564,7 +2760,12 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): # } # # A warning about missing end of namespace comments will be issued instead. - if IsBlankLine(line) and not nesting_state.InNamespaceBody(): + # + # Also skip blank line checks for 'extern "C"' blocks, which are formatted + # like namespaces. + if (IsBlankLine(line) and + not nesting_state.InNamespaceBody() and + not nesting_state.InExternC()): elided = clean_lines.elided prev_line = elided[linenum - 1] prevbrace = prev_line.rfind('{') @@ -2627,48 +2828,53 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): error(filename, linenum, 'whitespace/blank_line', 3, 'Do not leave a blank line after "%s:"' % matched.group(1)) - # Next, we complain if there's a comment too near the text - commentpos = line.find('//') - if commentpos != -1: - # Check if the // may be in quotes. If so, ignore it - # Comparisons made explicit for clarity -- pylint: disable=g-explicit-bool-comparison - if (line.count('"', 0, commentpos) - - line.count('\\"', 0, commentpos)) % 2 == 0: # not in quotes - # Allow one space for new scopes, two spaces otherwise: - if (not Match(r'^\s*{ //', line) and - ((commentpos >= 1 and - line[commentpos-1] not in string.whitespace) or - (commentpos >= 2 and - line[commentpos-2] not in string.whitespace))): - error(filename, linenum, 'whitespace/comments', 2, - 'At least two spaces is best between code and comments') - # There should always be a space between the // and the comment - commentend = commentpos + 2 - if commentend < len(line) and not line[commentend] == ' ': - # but some lines are exceptions -- e.g. if they're big - # comment delimiters like: - # //---------------------------------------------------------- - # or are an empty C++ style Doxygen comment, like: - # /// - # or C++ style Doxygen comments placed after the variable: - # ///< Header comment - # //!< Header comment - # or they begin with multiple slashes followed by a space: - # //////// Header comment - match = (Search(r'[=/-]{4,}\s*$', line[commentend:]) or - Search(r'^/$', line[commentend:]) or - Search(r'^!< ', line[commentend:]) or - Search(r'^/< ', line[commentend:]) or - Search(r'^/+ ', line[commentend:])) - if not match: - error(filename, linenum, 'whitespace/comments', 4, - 'Should have a space between // and comment') - CheckComment(line[commentpos:], filename, linenum, error) + # Next, check comments + next_line_start = 0 + if linenum + 1 < clean_lines.NumLines(): + next_line = raw[linenum + 1] + next_line_start = len(next_line) - len(next_line.lstrip()) + CheckComment(line, filename, linenum, next_line_start, error) - line = clean_lines.elided[linenum] # get rid of comments and strings + # get rid of comments and strings + line = clean_lines.elided[linenum] - # Don't try to do spacing checks for operator methods - line = re.sub(r'operator(==|!=|<|<<|<=|>=|>>|>)\(', 'operator\(', line) + # You shouldn't have spaces before your brackets, except maybe after + # 'delete []' or 'return []() {};' + if Search(r'\w\s+\[', line) and not Search(r'(?:delete|return)\s+\[', line): + error(filename, linenum, 'whitespace/braces', 5, + 'Extra space before [') + + # In range-based for, we wanted spaces before and after the colon, but + # not around "::" tokens that might appear. + if (Search(r'for *\(.*[^:]:[^: ]', line) or + Search(r'for *\(.*[^: ]:[^:]', line)): + error(filename, linenum, 'whitespace/forcolon', 2, + 'Missing space around colon in range-based for loop') + + +def CheckOperatorSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing around operators. + + 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. + """ + line = clean_lines.elided[linenum] + + # Don't try to do spacing checks for operator methods. Do this by + # replacing the troublesome characters with something else, + # preserving column position for all other characters. + # + # The replacement is done repeatedly to avoid false positives from + # operators that call operators. + while True: + match = Match(r'^(.*\boperator\b)(\S+)(\s*\(.*)$', line) + if match: + line = match.group(1) + ('_' * len(match.group(2))) + match.group(3) + else: + break # We allow no-spaces around = within an if: "if ( (a=Foo()) == 0 )". # Otherwise not. Note we only check for non-spaces on *both* sides; @@ -2686,42 +2892,51 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): # # Check <= and >= first to avoid false positives with < and >, then # check non-include lines for spacing around < and >. - match = Search(r'[^<>=!\s](==|!=|<=|>=)[^<>=!\s]', line) + # + # If the operator is followed by a comma, assume it's be used in a + # macro context and don't do any checks. This avoids false + # positives. + # + # Note that && is not included here. Those are checked separately + # in CheckRValueReference + match = Search(r'[^<>=!\s](==|!=|<=|>=|\|\|)[^<>=!\s,;\)]', line) if match: error(filename, linenum, 'whitespace/operators', 3, 'Missing spaces around %s' % match.group(1)) - # We allow no-spaces around << when used like this: 10<<20, but - # not otherwise (particularly, not when used as streams) - # Also ignore using ns::operator<<; - match = Search(r'(operator|\S)(?:L|UL|ULL|l|ul|ull)?<<(\S)', line) - if (match and - not (match.group(1).isdigit() and match.group(2).isdigit()) and - not (match.group(1) == 'operator' and match.group(2) == ';')): - error(filename, linenum, 'whitespace/operators', 3, - 'Missing spaces around <<') elif not Match(r'#.*include', line): - # Avoid false positives on -> - reduced_line = line.replace('->', '') - # Look for < that is not surrounded by spaces. This is only # triggered if both sides are missing spaces, even though # technically should should flag if at least one side is missing a # space. This is done to avoid some false positives with shifts. - match = Search(r'[^\s<]<([^\s=<].*)', reduced_line) - if (match and - not FindNextMatchingAngleBracket(clean_lines, linenum, match.group(1))): - error(filename, linenum, 'whitespace/operators', 3, - 'Missing spaces around <') + match = Match(r'^(.*[^\s<])<[^\s=<,]', line) + if match: + (_, _, end_pos) = CloseExpression( + clean_lines, linenum, len(match.group(1))) + if end_pos <= -1: + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around <') # Look for > that is not surrounded by spaces. Similar to the # above, we only trigger if both sides are missing spaces to avoid # false positives with shifts. - match = Search(r'^(.*[^\s>])>[^\s=>]', reduced_line) - if (match and - not FindPreviousMatchingAngleBracket(clean_lines, linenum, - match.group(1))): - error(filename, linenum, 'whitespace/operators', 3, - 'Missing spaces around >') + match = Match(r'^(.*[^-\s>])>[^\s=>,]', line) + if match: + (_, _, start_pos) = ReverseCloseExpression( + clean_lines, linenum, len(match.group(1))) + if start_pos <= -1: + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around >') + + # 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) + if (match and match.group(1) != '(' and + not (match.group(1).isdigit() and match.group(2).isdigit()) and + not (match.group(1) == 'operator' and match.group(2) == ';')): + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around <<') # We allow no-spaces around >> for almost anything. This is because # C++11 allows ">>" to close nested templates, which accounts for @@ -2746,7 +2961,19 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): error(filename, linenum, 'whitespace/operators', 4, 'Extra space for operator %s' % match.group(1)) - # A pet peeve of mine: no spaces after an if, while, switch, or for + +def CheckParenthesisSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing around parentheses. + + 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. + """ + line = clean_lines.elided[linenum] + + # No spaces after an if, while, switch, or for match = Search(r' (if\(|for\(|while\(|switch\()', line) if match: error(filename, linenum, 'whitespace/parens', 5, @@ -2772,6 +2999,19 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): 'Should have zero or one spaces inside ( and ) in %s' % match.group(1)) + +def CheckCommaSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing near commas and semicolons. + + 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. + """ + raw = clean_lines.lines_without_raw_strings + line = clean_lines.elided[linenum] + # You should always have a space after a comma (either as fn arg or operator) # # This does not apply when the non-space character following the @@ -2794,8 +3034,17 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): error(filename, linenum, 'whitespace/semicolon', 3, 'Missing space after ;') - # Next we will look for issues with function calls. - CheckSpacingForFunctionCall(filename, line, linenum, error) + +def CheckBracesSpacing(filename, clean_lines, linenum, error): + """Checks for horizontal spacing near commas. + + 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. + """ + line = clean_lines.elided[linenum] # Except after an opening paren, or after another opening brace (in case of # an initializer list, for instance), you should have spaces before your @@ -2812,10 +3061,12 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): # LastArgument(..., type{}); # LOG(INFO) << type{} << " ..."; # map_of_type[{...}] = ...; + # ternary = expr ? new type{} : nullptr; + # OuterTemplate{}> # # We check for the character following the closing brace, and # silence the warning if it's one of those listed above, i.e. - # "{.;,)<]". + # "{.;,)<>]:". # # To account for nested initializer list, we allow any number of # closing braces up to "{;,)<". We can't simply silence the @@ -2837,7 +3088,7 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): for offset in xrange(endlinenum + 1, min(endlinenum + 3, clean_lines.NumLines() - 1)): trailing_text += clean_lines.elided[offset] - if not Match(r'^[\s}]*[{.;,)<\]]', trailing_text): + if not Match(r'^[\s}]*[{.;,)<>\]:]', trailing_text): error(filename, linenum, 'whitespace/braces', 5, 'Missing space before {') @@ -2846,12 +3097,6 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): error(filename, linenum, 'whitespace/braces', 5, 'Missing space before else') - # You shouldn't have spaces before your brackets, except maybe after - # 'delete []' or 'new char * []'. - if Search(r'\w\s+\[', line) and not Search(r'delete\s+\[', line): - error(filename, linenum, 'whitespace/braces', 5, - 'Extra space before [') - # You shouldn't have a space before a semicolon at the end of the line. # There's a special case for "for" since the style guide allows space before # the semicolon there. @@ -2868,12 +3113,299 @@ def CheckSpacing(filename, clean_lines, linenum, nesting_state, error): 'Extra space before last semicolon. If this should be an empty ' 'statement, use {} instead.') - # In range-based for, we wanted spaces before and after the colon, but - # not around "::" tokens that might appear. - if (Search('for *\(.*[^:]:[^: ]', line) or - Search('for *\(.*[^: ]:[^:]', line)): - error(filename, linenum, 'whitespace/forcolon', 2, - 'Missing space around colon in range-based for loop') + +def IsDecltype(clean_lines, linenum, column): + """Check if the token ending on (linenum, column) is decltype(). + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: the number of the line to check. + column: end column of the token to check. + Returns: + True if this token is decltype() expression, False otherwise. + """ + (text, _, start_col) = ReverseCloseExpression(clean_lines, linenum, column) + if start_col < 0: + return False + if Search(r'\bdecltype\s*$', text[0:start_col]): + return True + return False + + +def IsTemplateParameterList(clean_lines, linenum, column): + """Check if the token ending on (linenum, column) is the end of template<>. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: the number of the line to check. + column: end column of the token to check. + Returns: + True if this token is end of a template parameter list, False otherwise. + """ + (_, startline, startpos) = ReverseCloseExpression( + clean_lines, linenum, column) + if (startpos > -1 and + Search(r'\btemplate\s*$', clean_lines.elided[startline][0:startpos])): + return True + return False + + +def IsRValueType(clean_lines, nesting_state, linenum, column): + """Check if the token ending on (linenum, column) is a type. + + Assumes that text to the right of the column is "&&" or a function + name. + + Args: + clean_lines: A CleansedLines instance containing the file. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. + linenum: the number of the line to check. + column: end column of the token to check. + Returns: + True if this token is a type, False if we are not sure. + """ + prefix = clean_lines.elided[linenum][0:column] + + # Get one word to the left. If we failed to do so, this is most + # likely not a type, since it's unlikely that the type name and "&&" + # would be split across multiple lines. + match = Match(r'^(.*)(\b\w+|[>*)&])\s*$', prefix) + if not match: + return False + + # Check text following the token. If it's "&&>" or "&&," or "&&...", it's + # most likely a rvalue reference used inside a template. + suffix = clean_lines.elided[linenum][column:] + if Match(r'&&\s*(?:[>,]|\.\.\.)', suffix): + return True + + # Check for simple type and end of templates: + # int&& variable + # vector&& variable + # + # Because this function is called recursively, we also need to + # recognize pointer and reference types: + # int* Function() + # int& Function() + if match.group(2) in ['char', 'char16_t', 'char32_t', 'wchar_t', 'bool', + 'short', 'int', 'long', 'signed', 'unsigned', + 'float', 'double', 'void', 'auto', '>', '*', '&']: + return True + + # If we see a close parenthesis, look for decltype on the other side. + # decltype would unambiguously identify a type, anything else is + # probably a parenthesized expression and not a type. + if match.group(2) == ')': + return IsDecltype( + clean_lines, linenum, len(match.group(1)) + len(match.group(2)) - 1) + + # Check for casts and cv-qualifiers. + # match.group(1) remainder + # -------------- --------- + # const_cast< type&& + # const type&& + # type const&& + if Search(r'\b(?:const_cast\s*<|static_cast\s*<|dynamic_cast\s*<|' + r'reinterpret_cast\s*<|\w+\s)\s*$', + match.group(1)): + return True + + # Look for a preceding symbol that might help differentiate the context. + # These are the cases that would be ambiguous: + # match.group(1) remainder + # -------------- --------- + # Call ( expression && + # Declaration ( type&& + # sizeof ( type&& + # if ( expression && + # while ( expression && + # for ( type&& + # for( ; expression && + # statement ; type&& + # block { type&& + # constructor { expression && + start = linenum + line = match.group(1) + match_symbol = None + while start >= 0: + # We want to skip over identifiers and commas to get to a symbol. + # Commas are skipped so that we can find the opening parenthesis + # for function parameter lists. + match_symbol = Match(r'^(.*)([^\w\s,])[\w\s,]*$', line) + if match_symbol: + break + start -= 1 + line = clean_lines.elided[start] + + if not match_symbol: + # Probably the first statement in the file is an rvalue reference + return True + + if match_symbol.group(2) == '}': + # Found closing brace, probably an indicate of this: + # block{} type&& + return True + + if match_symbol.group(2) == ';': + # Found semicolon, probably one of these: + # for(; expression && + # statement; type&& + + # Look for the previous 'for(' in the previous lines. + before_text = match_symbol.group(1) + for i in xrange(start - 1, max(start - 6, 0), -1): + before_text = clean_lines.elided[i] + before_text + if Search(r'for\s*\([^{};]*$', before_text): + # This is the condition inside a for-loop + return False + + # Did not find a for-init-statement before this semicolon, so this + # is probably a new statement and not a condition. + return True + + if match_symbol.group(2) == '{': + # Found opening brace, probably one of these: + # block{ type&& = ... ; } + # constructor{ expression && expression } + + # Look for a closing brace or a semicolon. If we see a semicolon + # first, this is probably a rvalue reference. + line = clean_lines.elided[start][0:len(match_symbol.group(1)) + 1] + end = start + depth = 1 + while True: + for ch in line: + if ch == ';': + return True + elif ch == '{': + depth += 1 + elif ch == '}': + depth -= 1 + if depth == 0: + return False + end += 1 + if end >= clean_lines.NumLines(): + break + line = clean_lines.elided[end] + # Incomplete program? + return False + + if match_symbol.group(2) == '(': + # Opening parenthesis. Need to check what's to the left of the + # parenthesis. Look back one extra line for additional context. + before_text = match_symbol.group(1) + if linenum > 1: + before_text = clean_lines.elided[linenum - 1] + before_text + before_text = match_symbol.group(1) + + # Patterns that are likely to be types: + # [](type&& + # for (type&& + # sizeof(type&& + # operator=(type&& + # + if Search(r'(?:\]|\bfor|\bsizeof|\boperator\s*\S+\s*)\s*$', before_text): + return True + + # Patterns that are likely to be expressions: + # if (expression && + # while (expression && + # : initializer(expression && + # , initializer(expression && + # ( FunctionCall(expression && + # + FunctionCall(expression && + # + (expression && + # + # The last '+' represents operators such as '+' and '-'. + if Search(r'(?:\bif|\bwhile|[-+=%^(]*>)?\s*$', + match_symbol.group(1)) + if match_func: + # Check for constructors, which don't have return types. + if Search(r'\bexplicit$', match_func.group(1)): + return True + implicit_constructor = Match(r'\s*(\w+)\((?:const\s+)?(\w+)', prefix) + if (implicit_constructor and + implicit_constructor.group(1) == implicit_constructor.group(2)): + return True + return IsRValueType(clean_lines, nesting_state, linenum, + len(match_func.group(1))) + + # Nothing before the function name. If this is inside a block scope, + # this is probably a function call. + return not (nesting_state.previous_stack_top and + nesting_state.previous_stack_top.IsBlockInfo()) + + if match_symbol.group(2) == '>': + # Possibly a closing bracket, check that what's on the other side + # looks like the start of a template. + return IsTemplateParameterList( + clean_lines, start, len(match_symbol.group(1))) + + # Some other symbol, usually something like "a=b&&c". This is most + # likely not a type. + return False + + +def IsRValueAllowed(clean_lines, linenum): + """Check if RValue reference is allowed within some range of lines. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + True if line is within the region where RValue references are allowed. + """ + for i in xrange(linenum, 0, -1): + line = clean_lines.elided[i] + if Match(r'GOOGLE_ALLOW_RVALUE_REFERENCES_(?:PUSH|POP)', line): + if not line.endswith('PUSH'): + return False + for j in xrange(linenum, clean_lines.NumLines(), 1): + line = clean_lines.elided[j] + if Match(r'GOOGLE_ALLOW_RVALUE_REFERENCES_(?:PUSH|POP)', line): + return line.endswith('POP') + return False + + +def CheckRValueReference(filename, clean_lines, linenum, nesting_state, error): + """Check for rvalue references. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + nesting_state: A NestingState instance which maintains information about + the current stack of nested blocks being parsed. + error: The function to call with any errors found. + """ + # Find lines missing spaces around &&. + # TODO(unknown): currently we don't check for rvalue references + # with spaces surrounding the && to avoid false positives with + # boolean expressions. + line = clean_lines.elided[linenum] + match = Match(r'^(.*\S)&&', line) + if not match: + match = Match(r'(.*)&&\S', line) + if (not match) or '(&&)' in line or Search(r'\boperator\s*$', match.group(1)): + return + + # Either poorly formed && or an rvalue reference, check the context + # to get a more accurate error message. Mostly we want to determine + # if what's to the left of "&&" is a type or not. + and_pos = len(match.group(1)) + if IsRValueType(clean_lines, nesting_state, linenum, and_pos): + if not IsRValueAllowed(clean_lines, linenum): + error(filename, linenum, 'build/c++11', 3, + 'RValue references are an unapproved C++ feature.') + else: + error(filename, linenum, 'whitespace/operators', 3, + 'Missing spaces around &&') def CheckSectionSpacing(filename, clean_lines, class_info, linenum, error): @@ -2981,7 +3513,7 @@ def CheckBraces(filename, clean_lines, linenum, error): '{ should almost always be at the end of the previous line') # An else clause should be on the same line as the preceding closing brace. - if Match(r'\s*else\s*', line): + if Match(r'\s*else\b\s*(?:if\b|\{|$)', line): prevline = GetPreviousNonBlankLine(clean_lines, linenum)[0] if Match(r'\s*}\s*$', prevline): error(filename, linenum, 'whitespace/newline', 4, @@ -2989,19 +3521,20 @@ def CheckBraces(filename, clean_lines, linenum, error): # If braces come on one side of an else, they should be on both. # However, we have to worry about "else if" that spans multiple lines! - if Search(r'}\s*else[^{]*$', line) or Match(r'[^}]*else\s*{', line): - if Search(r'}\s*else if([^{]*)$', line): # could be multi-line if - # find the ( after the if - pos = line.find('else if') - pos = line.find('(', pos) - if pos > 0: - (endline, _, endpos) = CloseExpression(clean_lines, linenum, pos) - if endline[endpos:].find('{') == -1: # must be brace after if - error(filename, linenum, 'readability/braces', 5, - 'If an else has a brace on one side, it should have it on both') - else: # common case: else not followed by a multi-line if - error(filename, linenum, 'readability/braces', 5, - 'If an else has a brace on one side, it should have it on both') + if Search(r'else if\s*\(', line): # could be multi-line if + brace_on_left = bool(Search(r'}\s*else if\s*\(', line)) + # find the ( after the if + pos = line.find('else if') + pos = line.find('(', pos) + if pos > 0: + (endline, _, endpos) = CloseExpression(clean_lines, linenum, pos) + brace_on_right = endline[endpos:].find('{') != -1 + if brace_on_left != brace_on_right: # must be brace after if + error(filename, linenum, 'readability/braces', 5, + 'If an else has a brace on one side, it should have it on both') + elif Search(r'}\s*else[^{]*$', line) or Match(r'[^}]*else\s*{', line): + error(filename, linenum, 'readability/braces', 5, + 'If an else has a brace on one side, it should have it on both') # Likewise, an else should never have the else clause on the same line if Search(r'\belse [^\s{]', line) and not Search(r'\belse if\b', line): @@ -3013,6 +3546,70 @@ def CheckBraces(filename, clean_lines, linenum, error): error(filename, linenum, 'whitespace/newline', 4, 'do/while clauses should not be on a single line') + # Check single-line if/else bodies. The style guide says 'curly braces are not + # required for single-line statements'. We additionally allow multi-line, + # single statements, but we reject anything with more than one semicolon in + # it. This means that the first semicolon after the if should be at the end of + # its line, and the line after that should have an indent level equal to or + # lower than the if. We also check for ambiguous if/else nesting without + # braces. + if_else_match = Search(r'\b(if\s*\(|else\b)', line) + if if_else_match and not Match(r'\s*#', line): + if_indent = GetIndentLevel(line) + endline, endlinenum, endpos = line, linenum, if_else_match.end() + if_match = Search(r'\bif\s*\(', line) + if if_match: + # This could be a multiline if condition, so find the end first. + pos = if_match.end() - 1 + (endline, endlinenum, endpos) = CloseExpression(clean_lines, linenum, pos) + # Check for an opening brace, either directly after the if or on the next + # line. If found, this isn't a single-statement conditional. + if (not Match(r'\s*{', endline[endpos:]) + and not (Match(r'\s*$', endline[endpos:]) + and endlinenum < (len(clean_lines.elided) - 1) + and Match(r'\s*{', clean_lines.elided[endlinenum + 1]))): + while (endlinenum < len(clean_lines.elided) + and ';' not in clean_lines.elided[endlinenum][endpos:]): + endlinenum += 1 + endpos = 0 + if endlinenum < len(clean_lines.elided): + endline = clean_lines.elided[endlinenum] + # We allow a mix of whitespace and closing braces (e.g. for one-liner + # 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') + elif endlinenum < len(clean_lines.elided) - 1: + # Make sure the next line is dedented + next_line = clean_lines.elided[endlinenum + 1] + next_indent = GetIndentLevel(next_line) + # With ambiguous nested if statements, this will error out on the + # if that *doesn't* match the else, regardless of whether it's the + # inner one or outer one. + if (if_match and Match(r'\s*else\b', next_line) + and next_indent != if_indent): + error(filename, linenum, 'readability/braces', 4, + 'Else clause should be indented at the same level as if. ' + 'Ambiguous nested if/else chains require braces.') + elif next_indent > if_indent: + error(filename, linenum, 'readability/braces', 4, + 'If/else bodies with multiple statements require braces') + + +def CheckTrailingSemicolon(filename, clean_lines, linenum, error): + """Looks for redundant trailing semicolon. + + 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. + """ + + line = clean_lines.elided[linenum] + # Block bodies should not be followed by a semicolon. Due to C++11 # brace initialization, there are more places where semicolons are # required than not, so we use a whitelist approach to check these @@ -3081,20 +3678,27 @@ def CheckBraces(filename, clean_lines, linenum, error): # would result in compile errors. # # In addition to macros, we also don't want to warn on compound - # literals. + # literals and lambdas. closing_brace_pos = match.group(1).rfind(')') opening_parenthesis = ReverseCloseExpression( clean_lines, linenum, closing_brace_pos) if opening_parenthesis[2] > -1: line_prefix = opening_parenthesis[0][0:opening_parenthesis[2]] macro = Search(r'\b([A-Z_]+)\s*$', line_prefix) + func = Match(r'^(.*\])\s*$', line_prefix) if ((macro and macro.group(1) not in ( 'TEST', 'TEST_F', 'MATCHER', 'MATCHER_P', 'TYPED_TEST', 'EXCLUSIVE_LOCKS_REQUIRED', 'SHARED_LOCKS_REQUIRED', 'LOCKS_EXCLUDED', 'INTERFACE_DEF')) or + (func and not Search(r'\boperator\s*\[\s*\]', func.group(1))) or Search(r'\s+=\s*$', line_prefix)): match = None + if (match and + opening_parenthesis[1] > 1 and + Search(r'\]\s*$', clean_lines.elided[opening_parenthesis[1] - 1])): + # Multi-line lambda-expression + match = None else: # Try matching cases 2-3. @@ -3163,6 +3767,29 @@ def CheckEmptyBlockBody(filename, clean_lines, linenum, error): 'Empty loop bodies should use {} or continue') +def FindCheckMacro(line): + """Find a replaceable CHECK-like macro. + + Args: + line: line to search on. + Returns: + (macro name, start position), or (None, -1) if no replaceable + macro is found. + """ + for macro in _CHECK_MACROS: + i = line.find(macro) + if i >= 0: + # Find opening parenthesis. Do a regular expression match here + # to make sure that we are matching the expected CHECK macro, as + # opposed to some other macro that happens to contain the CHECK + # substring. + matched = Match(r'^(.*\b' + macro + r'\s*)\(', line) + if not matched: + continue + return (macro, len(matched.group(1))) + return (None, -1) + + def CheckCheck(filename, clean_lines, linenum, error): """Checks the use of CHECK and EXPECT macros. @@ -3175,24 +3802,8 @@ def CheckCheck(filename, clean_lines, linenum, error): # Decide the set of replacement macros that should be suggested lines = clean_lines.elided - check_macro = None - start_pos = -1 - for macro in _CHECK_MACROS: - i = lines[linenum].find(macro) - if i >= 0: - check_macro = macro - - # Find opening parenthesis. Do a regular expression match here - # to make sure that we are matching the expected CHECK macro, as - # opposed to some other macro that happens to contain the CHECK - # substring. - matched = Match(r'^(.*\b' + check_macro + r'\s*)\(', lines[linenum]) - if not matched: - continue - start_pos = len(matched.group(1)) - break - if not check_macro or start_pos < 0: - # Don't waste time here if line doesn't contain 'CHECK' or 'EXPECT' + (check_macro, start_pos) = FindCheckMacro(lines[linenum]) + if not check_macro: return # Find end of the boolean expression by matching parentheses @@ -3222,7 +3833,7 @@ def CheckCheck(filename, clean_lines, linenum, error): if token == '(': # Parenthesized operand expression = matched.group(2) - (end, _) = FindEndOfExpressionInLine(expression, 0, 1, '(', ')') + (end, _) = FindEndOfExpressionInLine(expression, 0, ['(']) if end < 0: return # Unmatched parenthesis lhs += '(' + expression[0:end] @@ -3357,7 +3968,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, 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. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: The function to call with any errors found. """ @@ -3384,6 +3995,8 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, # if(match($0, " <<")) complain = 0; # if(match(prev, " +for \\(")) complain = 0; # if(prevodd && match(prevprev, " +for \\(")) complain = 0; + scope_or_label_pattern = r'\s*\w+\s*:\s*\\?$' + classinfo = nesting_state.InnermostClass() initial_spaces = 0 cleansed_line = clean_lines.elided[linenum] while initial_spaces < len(line) and line[initial_spaces] == ' ': @@ -3391,9 +4004,12 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, if line and line[-1].isspace(): error(filename, linenum, 'whitespace/end_of_line', 4, 'Line ends in whitespace. Consider deleting these extra spaces.') - # There are certain situations we allow one space, notably for section labels + # There are certain situations we allow one space, notably for + # section labels, and also lines containing multi-line raw strings. elif ((initial_spaces == 1 or initial_spaces == 3) and - not Match(r'\s*\w+\s*:\s*$', cleansed_line)): + not Match(scope_or_label_pattern, cleansed_line) and + not (clean_lines.raw_lines[linenum] != line and + Match(r'^\s*""', line))): error(filename, linenum, 'whitespace/indent', 3, 'Weird number of spaces at line-start. ' 'Are you using a 2-space indent?') @@ -3441,9 +4057,16 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, # Some more style checks CheckBraces(filename, clean_lines, linenum, error) + CheckTrailingSemicolon(filename, clean_lines, linenum, error) CheckEmptyBlockBody(filename, clean_lines, linenum, error) CheckAccess(filename, clean_lines, linenum, nesting_state, error) CheckSpacing(filename, clean_lines, linenum, nesting_state, error) + CheckOperatorSpacing(filename, clean_lines, linenum, error) + CheckParenthesisSpacing(filename, clean_lines, linenum, error) + CheckCommaSpacing(filename, clean_lines, linenum, error) + CheckBracesSpacing(filename, clean_lines, linenum, error) + CheckSpacingForFunctionCall(filename, clean_lines, linenum, error) + CheckRValueReference(filename, clean_lines, linenum, nesting_state, error) CheckCheck(filename, clean_lines, linenum, error) CheckAltTokens(filename, clean_lines, linenum, error) classinfo = nesting_state.InnermostClass() @@ -3580,7 +4203,6 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): error: The function to call with any errors found. """ fileinfo = FileInfo(filename) - line = clean_lines.lines[linenum] # "include" should use the new style "foo/bar.h" instead of just "bar.h" @@ -3633,8 +4255,13 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): 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.') + # Suggest a different header for ostream + if include == 'ostream': + error(filename, linenum, 'readability/streams', 3, + 'For logging, include "base/logging.h" instead of .') + else: + error(filename, linenum, 'readability/streams', 3, + 'Streams are highly discouraged.') def _GetTextInside(text, start_pattern): @@ -3657,7 +4284,7 @@ def _GetTextInside(text, start_pattern): The extracted text. None if either the opening string or ending punctuation could not be found. """ - # TODO(sugawarayu): Audit cpplint.py to see what places could be profitably + # TODO(unknown): Audit cpplint.py to see what places could be profitably # rewritten to use _GetTextInside (and use inferior regexp matching today). # Give opening punctuations to get the matching close-punctuations. @@ -3732,7 +4359,7 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, 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. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: The function to call with any errors found. """ @@ -3754,118 +4381,11 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, # Make Windows paths like Unix. fullname = os.path.abspath(filename).replace('\\', '/') - - # TODO(unknown): figure out if they're using default arguments in fn proto. - - # Check to see if they're using an conversion function cast. - # I just try to capture the most common basic types, though there are more. - # Parameterless conversion functions, such as bool(), are allowed as they are - # probably a member operator declaration or default constructor. - match = Search( - r'(\bnew\s+)?\b' # Grab 'new' operator, if it's there - r'(int|float|double|bool|char|int32|uint32|int64|uint64)' - r'(\([^)].*)', line) - if match: - matched_new = match.group(1) - matched_type = match.group(2) - matched_funcptr = match.group(3) - - # 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. 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. - # - # std::function<> wrapper has a similar problem. - # - # Return types for function pointers also look like casts if they - # don't have an extra space. - if (matched_new is None and # If new operator, then this isn't a cast - not (Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line) or - Search(r'\bMockCallback<.*>', line) or - Search(r'\bstd::function<.*>', line)) and - not (matched_funcptr and - Match(r'\((?:[^() ]+::\s*\*\s*)?[^() ]+\)\s*\(', - matched_funcptr))): - # Try a bit harder to catch gmock lines: the only place where - # something looks like an old-style cast is where we declare the - # return type of the mocked method, and the only time when we - # are missing context is if MOCK_METHOD was split across - # multiple lines. The missing MOCK_METHOD is usually one or two - # lines back, so scan back one or two lines. - # - # It's not possible for gmock macros to appear in the first 2 - # lines, since the class head + section name takes up 2 lines. - if (linenum < 2 or - not (Match(r'^\s*MOCK_(?:CONST_)?METHOD\d+(?:_T)?\((?:\S+,)?\s*$', - clean_lines.elided[linenum - 1]) or - Match(r'^\s*MOCK_(?:CONST_)?METHOD\d+(?:_T)?\(\s*$', - clean_lines.elided[linenum - 2]))): - error(filename, linenum, 'readability/casting', 4, - 'Using deprecated casting style. ' - 'Use static_cast<%s>(...) instead' % - matched_type) - - CheckCStyleCast(filename, linenum, line, clean_lines.raw_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): - 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) - - # 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. - match = Search( - r'(?:&\(([^)]+)\)[\w(])|' - r'(?:&(static|dynamic|down|reinterpret)_cast\b)', line) - if match and match.group(1) != '*': - error(filename, linenum, 'runtime/casting', 4, - ('Are you taking an address of a cast? ' - 'This is dangerous: could be a temp var. ' - 'Take the address before doing the cast, rather than after')) - - # 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. - if linenum + 1 < clean_lines.NumLines(): - extended_line = line + clean_lines.elided[linenum + 1] - else: - extended_line = line - - # 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) - # Make sure it's not a function. - # Function template specialization looks like: "string foo(...". - # Class template definitions look like: "string Foo::Method(...". - # - # Also ignore things that look like operators. These are matched separately - # because operator names cross non-word boundaries. If we change the pattern - # above, we would decrease the accuracy of matching identifiers. - if (match and - not Search(r'\boperator\W', line) and - 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[]".' % - (match.group(1), match.group(2))) - - if Search(r'\b([A-Za-z0-9_]*_)\(\1\)', line): - error(filename, linenum, 'runtime/init', 4, - 'You seem to be initializing a member variable with itself.') + + # Perform other checks now that we are sure that this is not an include line + CheckCasts(filename, clean_lines, linenum, error) + CheckGlobalStatic(filename, clean_lines, linenum, error) + CheckPrintf(filename, clean_lines, linenum, error) if file_extension == 'h': # TODO(unknown): check that 1-arg constructors are explicit. @@ -3887,23 +4407,6 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, error(filename, linenum, 'runtime/int', 4, 'Use int16/int64/etc, rather than the C type %s' % match.group(1)) - # When snprintf is used, the second argument shouldn't be a literal. - match = Search(r'snprintf\s*\(([^,]*),\s*([0-9]*)\s*,', line) - if match and match.group(2) != '0': - # If 2nd arg is zero, snprintf is used to calculate size. - error(filename, linenum, 'runtime/printf', 3, - 'If you can, use sizeof(%s) instead of %s as the 2nd arg ' - 'to snprintf.' % (match.group(1), match.group(2))) - - # Check if some verboten C functions are being used. - if Search(r'\bsprintf\b', line): - error(filename, linenum, 'runtime/printf', 5, - 'Never use sprintf. Use snprintf instead.') - match = Search(r'\b(strcpy|strcat)\b', line) - if match: - error(filename, linenum, 'runtime/printf', 4, - 'Almost always, snprintf is better than %s' % match.group(1)) - # Check if some verboten operator overloading is going on # TODO(unknown): catch out-of-line unary operator&: # class X {}; @@ -3923,7 +4426,7 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, # Check for potential format string bugs like printf(foo). # We constrain the pattern not to pick things like DocidForPrintf(foo). # Not perfect but it can catch printf(foo.c_str()) and printf(foo->c_str()) - # TODO(sugawarayu): Catch the following case. Need to change the calling + # TODO(unknown): Catch the following case. Need to change the calling # convention of the whole function to process multiple line to handle it. # printf( # boy_this_is_a_really_long_variable_that_cannot_fit_on_the_prev_line); @@ -4019,6 +4522,144 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, 'http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Namespaces' ' for more information.') + +def CheckGlobalStatic(filename, clean_lines, linenum, error): + """Check for unsafe global or static objects. + + 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. + """ + line = clean_lines.elided[linenum] + + # 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 + # const string *pointer + # string const *pointer + # string *const pointer + # + # - Functions and template specializations. + # string Function(... + # string Class::Method(... + # + # - Operators. These are matched separately because operator names + # cross non-word boundaries, and trying to match both operators + # and functions at the same time would decrease accuracy of + # matching identifiers. + # string Class::operator*() + 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))): + error(filename, linenum, 'runtime/string', 4, + 'For a static/global string constant, use a C style string instead: ' + '"%schar %s[]".' % + (match.group(1), match.group(2))) + + if Search(r'\b([A-Za-z0-9_]*_)\(\1\)', line): + error(filename, linenum, 'runtime/init', 4, + 'You seem to be initializing a member variable with itself.') + + +def CheckPrintf(filename, clean_lines, linenum, error): + """Check for printf related issues. + + 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. + """ + line = clean_lines.elided[linenum] + + # When snprintf is used, the second argument shouldn't be a literal. + match = Search(r'snprintf\s*\(([^,]*),\s*([0-9]*)\s*,', line) + if match and match.group(2) != '0': + # If 2nd arg is zero, snprintf is used to calculate size. + error(filename, linenum, 'runtime/printf', 3, + 'If you can, use sizeof(%s) instead of %s as the 2nd arg ' + 'to snprintf.' % (match.group(1), match.group(2))) + + # Check if some verboten C functions are being used. + if Search(r'\bsprintf\b', line): + error(filename, linenum, 'runtime/printf', 5, + 'Never use sprintf. Use snprintf instead.') + match = Search(r'\b(strcpy|strcat)\b', line) + if match: + error(filename, linenum, 'runtime/printf', 4, + 'Almost always, snprintf is better than %s' % match.group(1)) + + +def IsDerivedFunction(clean_lines, linenum): + """Check if current line contains an inherited function. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + 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:]) + + +def IsInitializerList(clean_lines, linenum): + """Check if current line is inside constructor initializer list. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + True if current line appears to be inside constructor initializer + list, False otherwise. + """ + for i in xrange(linenum, 1, -1): + line = clean_lines.elided[i] + if i == linenum: + remove_function_body = Match(r'^(.*)\{\s*$', line) + if remove_function_body: + line = remove_function_body.group(1) + + if Search(r'\s:\s*\w+[({]', line): + # A lone colon tend to indicate the start of a constructor + # initializer list. It could also be a ternary operator, which + # also tend to appear in constructor initializer lists as + # opposed to parameter lists. + return True + if Search(r'\}\s*,\s*$', line): + # A closing brace followed by a comma is probably the end of a + # brace-initialized member in constructor initializer list. + return True + if Search(r'[{};]\s*$', line): + # Found one of the following: + # - A closing brace or semicolon, probably the end of the previous + # function. + # - An opening brace, probably the start of current class or namespace. + # + # Current line is probably not inside an initializer list since + # we saw one of those things without seeing the starting colon. + return False + + # Got to the beginning of the file without seeing the start of + # constructor initializer list. + return False + + def CheckForNonConstReference(filename, clean_lines, linenum, nesting_state, error): """Check for non-const references. @@ -4030,7 +4671,7 @@ def CheckForNonConstReference(filename, clean_lines, linenum, filename: The name of the current file. clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: The function to call with any errors found. """ @@ -4039,6 +4680,12 @@ def CheckForNonConstReference(filename, clean_lines, linenum, if '&' not in line: return + # If a function is inherited, current function doesn't have much of + # a choice, so any non-const references should not be blamed on + # derived function. + if IsDerivedFunction(clean_lines, linenum): + return + # Long type names may be broken across multiple lines, usually in one # of these forms: # LongType @@ -4087,19 +4734,21 @@ def CheckForNonConstReference(filename, clean_lines, linenum, # inside declarators: reference parameter # We will exclude the first two cases by checking that we are not inside a # function body, including one that was just introduced by a trailing '{'. - # TODO(unknwon): Doesn't account for preprocessor directives. # TODO(unknown): Doesn't account for 'catch(Exception& e)' [rare]. - check_params = False - if not nesting_state.stack: - check_params = True # top level - elif (isinstance(nesting_state.stack[-1], _ClassInfo) or - isinstance(nesting_state.stack[-1], _NamespaceInfo)): - check_params = True # within class or namespace - elif Match(r'.*{\s*$', line): - if (len(nesting_state.stack) == 1 or - isinstance(nesting_state.stack[-2], _ClassInfo) or - isinstance(nesting_state.stack[-2], _NamespaceInfo)): - check_params = True # just opened global/class/namespace block + if (nesting_state.previous_stack_top and + not (isinstance(nesting_state.previous_stack_top, _ClassInfo) or + isinstance(nesting_state.previous_stack_top, _NamespaceInfo))): + # Not at toplevel, not within a class, and not within a namespace + return + + # Avoid preprocessors + if Search(r'\\\s*$', line): + return + + # Avoid constructor initializer lists + if IsInitializerList(clean_lines, linenum): + return + # We allow non-const references in a few standard places, like functions # called "swap()" or iostream operators like "<<" or ">>". Do not check # those function parameters. @@ -4111,7 +4760,7 @@ def CheckForNonConstReference(filename, clean_lines, linenum, r'static_assert|COMPILE_ASSERT' r')\s*\(') if Search(whitelisted_functions, line): - check_params = False + return elif not Search(r'\S+\([^)]*$', line): # Don't see a whitelisted function on this line. Actually we # didn't see any function name on this line, so this is likely a @@ -4119,17 +4768,122 @@ def CheckForNonConstReference(filename, clean_lines, linenum, for i in xrange(2): if (linenum > i and Search(whitelisted_functions, clean_lines.elided[linenum - i - 1])): - check_params = False - break + return - if check_params: - decls = ReplaceAll(r'{[^}]*}', ' ', line) # exclude function body - for parameter in re.findall(_RE_PATTERN_REF_PARAM, decls): - if not Match(_RE_PATTERN_CONST_REF_PARAM, parameter): - error(filename, linenum, 'runtime/references', 2, - 'Is this a non-const reference? ' - 'If so, make const or use a pointer: ' + - ReplaceAll(' *<', '<', parameter)) + decls = ReplaceAll(r'{[^}]*}', ' ', line) # exclude function body + for parameter in re.findall(_RE_PATTERN_REF_PARAM, decls): + if not Match(_RE_PATTERN_CONST_REF_PARAM, parameter): + error(filename, linenum, 'runtime/references', 2, + 'Is this a non-const reference? ' + 'If so, make const or use a pointer: ' + + ReplaceAll(' *<', '<', parameter)) + + +def CheckCasts(filename, clean_lines, linenum, error): + """Various cast related checks. + + 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. + """ + line = clean_lines.elided[linenum] + + # Check to see if they're using an conversion function cast. + # I just try to capture the most common basic types, though there are more. + # Parameterless conversion functions, such as bool(), are allowed as they are + # probably a member operator declaration or default constructor. + match = Search( + r'(\bnew\s+|\S<\s*(?:const\s+)?)?\b' + r'(int|float|double|bool|char|int32|uint32|int64|uint64)' + r'(\([^)].*)', line) + expecting_function = ExpectingFunctionArgs(clean_lines, linenum) + if match and not expecting_function: + matched_type = match.group(2) + + # matched_new_or_template is used to silence two false positives: + # - New operators + # - Template arguments with function types + # + # For template arguments, we match on types immediately following + # an opening bracket without any spaces. This is a fast way to + # silence the common case where the function type is the first + # template argument. False negative with less-than comparison is + # avoided because those operators are usually followed by a space. + # + # function // bracket + no space = false positive + # value < double(42) // bracket + space = true positive + matched_new_or_template = match.group(1) + + # Other things to ignore: + # - Function pointers + # - Casts to pointer types + # - Placement new + # - Alias declarations + matched_funcptr = match.group(3) + if (matched_new_or_template is None and + not (matched_funcptr and + (Match(r'\((?:[^() ]+::\s*\*\s*)?[^() ]+\)\s*\(', + matched_funcptr) or + matched_funcptr.startswith('(*)'))) and + not Match(r'\s*using\s+\S+\s*=\s*' + matched_type, line) and + not Search(r'new\(\S+\)\s*' + matched_type, line)): + error(filename, linenum, 'readability/casting', 4, + 'Using deprecated casting style. ' + 'Use static_cast<%s>(...) instead' % + matched_type) + + if not expecting_function: + CheckCStyleCast(filename, linenum, line, clean_lines.raw_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): + 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) + + # 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. + match = Search( + r'(?:&\(([^)]+)\)[\w(])|' + r'(?:&(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 + # pointer itself. + parenthesis_error = False + match = Match(r'^(.*&(?:static|dynamic|down|reinterpret)_cast\b)<', line) + if match: + _, y1, x1 = CloseExpression(clean_lines, linenum, len(match.group(1))) + if x1 >= 0 and clean_lines.elided[y1][x1] == '(': + _, y2, x2 = CloseExpression(clean_lines, y1, x1) + if x2 >= 0: + extended_line = clean_lines.elided[y2][x2:] + if y2 < clean_lines.NumLines() - 1: + extended_line += clean_lines.elided[y2 + 1] + if Match(r'\s*(?:->|\[)', extended_line): + parenthesis_error = True + + if parenthesis_error: + error(filename, linenum, 'readability/casting', 4, + ('Are you taking an address of something dereferenced ' + 'from a cast? Wrapping the dereferenced expression in ' + 'parentheses will make the binding more obvious')) + else: + error(filename, linenum, 'runtime/casting', 4, + ('Are you taking an address of a cast? ' + 'This is dangerous: could be a temp var. ' + 'Take the address before doing the cast, rather than after')) def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, @@ -4154,9 +4908,10 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, if not match: return False - # Exclude lines with sizeof, since sizeof looks like a cast. - sizeof_match = Match(r'.*sizeof\s*$', line[0:match.start(1) - 1]) - if sizeof_match: + # 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]): return False # operator++(int) and operator--(int) @@ -4188,7 +4943,8 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, # ; # <(FunctionPointerTemplateArgument)(int)>; remainder = line[match.end(0):] - if Match(r'^\s*(?:;|const\b|throw\b|=|>|\{|\))', remainder): + if Match(r'^\s*(?:;|const\b|throw\b|final\b|override\b|=|>|\{|\))', + remainder): # Looks like an unnamed parameter. # Don't warn on any kind of template arguments. @@ -4226,6 +4982,28 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, return True +def ExpectingFunctionArgs(clean_lines, linenum): + """Checks whether where function type arguments are expected. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + + Returns: + True if the line at 'linenum' is inside something that expects arguments + of function types. + """ + line = clean_lines.elided[linenum] + return (Match(r'^\s*MOCK_(CONST_)?METHOD\d+(_T)?\(', line) or + (linenum >= 2 and + (Match(r'^\s*MOCK_(?:CONST_)?METHOD\d+(?:_T)?\((?:\S+,)?\s*$', + clean_lines.elided[linenum - 1]) or + Match(r'^\s*MOCK_(?:CONST_)?METHOD\d+(?:_T)?\(\s*$', + clean_lines.elided[linenum - 2]) or + Search(r'\bstd::m?function\s*\<\s*$', + clean_lines.elided[linenum - 1])))) + + _HEADERS_CONTAINING_TEMPLATES = ( ('', ('deque',)), ('', ('unary_function', 'binary_function', @@ -4467,7 +5245,7 @@ _RE_PATTERN_EXPLICIT_MAKEPAIR = re.compile(r'\bmake_pair\s*<') def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error): """Check that make_pair's template arguments are deduced. - G++ 4.6 in C++0x mode fails badly if make_pair's template arguments are + G++ 4.6 in C++11 mode fails badly if make_pair's template arguments are specified explicitly, and such use isn't intended in any case. Args: @@ -4483,11 +5261,35 @@ 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. + + 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. + """ + line = clean_lines.elided[linenum] + + # A lambda introducer specifies a default capture if it starts with "[=" + # or if it starts with "[&" _not_ followed by an identifier. + match = Match(r'^(.*)\[\s*(?:=|&[^\w])', line) + if match: + # Found a potential error, check what comes after the lambda-introducer. + # If it's not open parenthesis (for lambda-declarator) or open brace + # (for compound-statement), it's not a lambda. + line, _, pos = CloseExpression(clean_lines, linenum, len(match.group(1))) + if pos >= 0 and Match(r'^\s*[{(]', line[pos:]): + error(filename, linenum, 'build/c++11', + 4, # 4 = high confidence + 'Default lambda captures are an unapproved C++ feature.') + + def ProcessLine(filename, file_extension, clean_lines, line, - include_state, function_state, nesting_state, error, - extra_check_functions=[]): + include_state, function_state, nesting_state, error): """Processes a single line in the file. Args: @@ -4498,19 +5300,16 @@ def ProcessLine(filename, file_extension, clean_lines, line, line: Number of line being processed. include_state: An _IncludeState instance in which the headers are inserted. function_state: A _FunctionState instance which counts function lines, etc. - nesting_state: A _NestingState instance which maintains information about + nesting_state: A NestingState instance which maintains information about the current stack of nested blocks being parsed. error: A callable to which errors are reported, which takes 4 arguments: filename, line number, error level, and message - extra_check_functions: An array of additional check functions that will be - run on each source line. Each function takes 4 - arguments: filename, clean_lines, line, error + """ raw_lines = clean_lines.raw_lines ParseNolintSuppressions(filename, raw_lines[line], line, error) nesting_state.Update(filename, clean_lines, line, error) - if nesting_state.stack and nesting_state.stack[-1].inline_asm != _NO_ASM: - return + if nesting_state.InAsmBlock(): return CheckForFunctionLengths(filename, clean_lines, line, function_state, error) CheckForMultilineCommentsAndStrings(filename, clean_lines, line, error) CheckStyle(filename, clean_lines, line, file_extension, nesting_state, error) @@ -4523,11 +5322,59 @@ def ProcessLine(filename, file_extension, clean_lines, line, CheckPosixThreading(filename, clean_lines, line, error) CheckInvalidIncrement(filename, clean_lines, line, error) CheckMakePairUsesDeduction(filename, clean_lines, line, error) - for check_fn in extra_check_functions: - check_fn(filename, clean_lines, line, error) + CheckDefaultLambdaCaptures(filename, clean_lines, line, error) -def ProcessFileData(filename, file_extension, lines, error, - extra_check_functions=[]): + +def FlagCxx11Features(filename, clean_lines, linenum, error): + """Flag those c++11 features that we only allow in certain places. + + 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. + """ + line = clean_lines.elided[linenum] + + # Flag unapproved C++11 headers. + include = Match(r'\s*#\s*include\s+[<"]([^<"]+)[">]', line) + if include and include.group(1) in ('cfenv', + 'condition_variable', + 'fenv.h', + 'future', + 'mutex', + 'thread', + 'chrono', + 'ratio', + 'regex', + 'system_error', + ): + error(filename, linenum, 'build/c++11', 5, + ('<%s> is an unapproved C++11 header.') % include.group(1)) + + # The only place where we need to worry about C++11 keywords and library + # features in preprocessor directives is in macro definitions. + if Match(r'\s*#', line) and not Match(r'\s*#\s*define\b', line): return + + # These are classes and free functions. The classes are always + # mentioned as std::*, but we only catch the free functions if + # they're not found by ADL. They're alphabetical by header. + for top_name in ( + # type_traits + 'alignment_of', + 'aligned_union', + + # utility + 'forward', + ): + if Search(r'\bstd::%s\b' % top_name, line): + error(filename, linenum, 'build/c++11', 5, + ('std::%s is an unapproved C++11 class or function. Send c-style ' + 'an example of where it would make your code more readable, and ' + 'they may let you use it.') % top_name) + + +def ProcessFileData(filename, file_extension, lines, error): """Performs lint checks and reports any errors to the given error function. Args: @@ -4536,17 +5383,13 @@ def ProcessFileData(filename, file_extension, lines, error, lines: An array of strings, each representing a line of the file, with the last element being empty if the file is terminated with a newline. error: A callable to which errors are reported, which takes 4 arguments: - filename, line number, error level, and message - extra_check_functions: An array of additional check functions that will be - run on each source line. Each function takes 4 - arguments: filename, clean_lines, line, error """ lines = (['// marker so line numbers and indices both start at 1'] + lines + ['// marker so line numbers end in a known way']) include_state = _IncludeState() function_state = _FunctionState() - nesting_state = _NestingState() + nesting_state = NestingState() ResetNolintSuppressions() @@ -4559,8 +5402,8 @@ def ProcessFileData(filename, file_extension, lines, error, clean_lines = CleansedLines(lines) for line in xrange(clean_lines.NumLines()): ProcessLine(filename, file_extension, clean_lines, line, - include_state, function_state, nesting_state, error, - extra_check_functions) + include_state, function_state, nesting_state, error) + FlagCxx11Features(filename, clean_lines, line, error) nesting_state.CheckCompletedBlocks(filename, error) CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error) @@ -4571,7 +5414,8 @@ def ProcessFileData(filename, file_extension, lines, error, CheckForNewlineAtEOF(filename, lines, error) -def ProcessFile(filename, vlevel, extra_check_functions=[]): + +def ProcessFile(filename, vlevel): """Does google-lint on a single file. Args: @@ -4579,14 +5423,12 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): vlevel: The level of errors to report. Every error of confidence >= verbose_level will be reported. 0 is a good default. - - extra_check_functions: An array of additional check functions that will be - run on each source line. Each function takes 4 - arguments: filename, clean_lines, line, error """ _SetVerboseLevel(vlevel) + lf_lines = [] + crlf_lines = [] try: # Support the UNIX convention of using "-" for stdin. Note that # we are not opening the file with universal newline support @@ -4594,10 +5436,7 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): # contain trailing '\r' characters if we are reading a file that # has CRLF endings. # If after the split a trailing '\r' is present, it is removed - # below. If it is not expected to be present (i.e. os.linesep != - # '\r\n' as in Windows), a warning is issued below if this file - # is processed. - + # below. if filename == '-': lines = codecs.StreamReaderWriter(sys.stdin, codecs.getreader('utf8'), @@ -4606,12 +5445,14 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): else: lines = codecs.open(filename, 'r', 'utf8', 'replace').read().split('\n') - carriage_return_found = False # Remove trailing '\r'. - for linenum in range(len(lines)): + # The -1 accounts for the extra trailing blank line we get from split() + for linenum in range(len(lines) - 1): if lines[linenum].endswith('\r'): lines[linenum] = lines[linenum].rstrip('\r') - carriage_return_found = True + crlf_lines.append(linenum + 1) + else: + lf_lines.append(linenum + 1) except IOError: sys.stderr.write( @@ -4627,14 +5468,25 @@ def ProcessFile(filename, vlevel, extra_check_functions=[]): sys.stderr.write('Ignoring %s; not a valid file name ' '(%s)\n' % (filename, ', '.join(_valid_extensions))) else: - ProcessFileData(filename, file_extension, lines, Error, - extra_check_functions) - if carriage_return_found and os.linesep != '\r\n': - # Use 0 for linenum since outputting only one error for potentially - # several lines. - Error(filename, 0, 'whitespace/newline', 1, - 'One or more unexpected \\r (^M) found;' - 'better to use only a \\n') + ProcessFileData(filename, file_extension, lines, Error) + + # If end-of-line sequences are a mix of LF and CR-LF, issue + # warnings on the lines with CR. + # + # Don't issue any warnings if all lines are uniformly LF or CR-LF, + # since critique can handle these just fine, and the style guide + # doesn't dictate a particular end of line sequence. + # + # We can't depend on os.linesep to determine what the desired + # end-of-line sequence should be, since that will return the + # server-side end-of-line sequence. + if lf_lines and crlf_lines: + # Warn on every line with CR. An alternative approach might be to + # check whether the file is mostly CRLF or just LF, and warn on the + # minority, we bias toward LF here since most tools prefer LF. + for linenum in crlf_lines: + Error(filename, linenum, 'whitespace/newline', 1, + 'Unexpected \\r (^M) found; better to use only \\n') sys.stderr.write('Done processing %s\n' % filename) diff --git a/cpplint/cpplint_unittest.py b/cpplint/cpplint_unittest.py index 253bb80..8a66def 100755 --- a/cpplint/cpplint_unittest.py +++ b/cpplint/cpplint_unittest.py @@ -97,6 +97,7 @@ class ErrorCollector: # 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 @@ -116,7 +117,7 @@ class CpplintTestBase(unittest.TestCase): clean_lines = cpplint.CleansedLines(lines) include_state = cpplint._IncludeState() function_state = cpplint._FunctionState() - nesting_state = cpplint._NestingState() + nesting_state = cpplint.NestingState() cpplint.ProcessLine('foo.cc', 'cc', clean_lines, 0, include_state, function_state, nesting_state, error_collector) @@ -132,7 +133,7 @@ class CpplintTestBase(unittest.TestCase): lines = code.split('\n') cpplint.RemoveMultiLineComments('foo.h', lines, error_collector) lines = cpplint.CleansedLines(lines) - nesting_state = cpplint._NestingState() + nesting_state = cpplint.NestingState() for i in xrange(lines.NumLines()): nesting_state.Update('foo.h', lines, i, error_collector) cpplint.CheckStyle('foo.h', lines, i, 'h', nesting_state, @@ -147,7 +148,7 @@ class CpplintTestBase(unittest.TestCase): def PerformLanguageRulesCheck(self, file_name, code): error_collector = ErrorCollector(self.assert_) include_state = cpplint._IncludeState() - nesting_state = cpplint._NestingState() + nesting_state = cpplint.NestingState() lines = code.split('\n') cpplint.RemoveMultiLineComments(file_name, lines, error_collector) lines = cpplint.CleansedLines(lines) @@ -187,7 +188,7 @@ class CpplintTestBase(unittest.TestCase): # First, build up the include state. error_collector = ErrorCollector(self.assert_) include_state = cpplint._IncludeState() - nesting_state = cpplint._NestingState() + nesting_state = cpplint.NestingState() lines = code.split('\n') cpplint.RemoveMultiLineComments(filename, lines, error_collector) lines = cpplint.CleansedLines(lines) @@ -262,7 +263,7 @@ class CpplintTest(CpplintTestBase): self.assertEquals('\n return loop(x);\n', cpplint._GetTextInside( 'int loop(int x) {\n return loop(x);\n}\n', r'\{')) - # '^' matches the beggining of each line. + # '^' matches the beginning of each line. self.assertEquals('x, y', cpplint._GetTextInside( '#include "inl.h" // skip #define\n' @@ -328,6 +329,22 @@ class CpplintTest(CpplintTestBase): '// $Id: g' + ('o' * 80) + 'gle.cc#1', 'Lines should be <= 80 characters long' ' [whitespace/line_length] [2]') + self.TestMultiLineLint( + 'static const char kCStr[] = "g' + ('o' * 50) + 'gle";\n', + 'Lines should be <= 80 characters long' + ' [whitespace/line_length] [2]') + self.TestMultiLineLint( + 'static const char kRawStr[] = R"(g' + ('o' * 50) + 'gle)";\n', + '') # no warning because raw string content is elided + self.TestMultiLineLint( + 'static const char kMultiLineRawStr[] = R"(\n' + 'g' + ('o' * 80) + 'gle\n' + ')";', + '') + self.TestMultiLineLint( + 'static const char kL' + ('o' * 50) + 'ngIdentifier[] = R"()";\n', + 'Lines should be <= 80 characters long' + ' [whitespace/line_length] [2]') # Test error suppression annotations. def testErrorSuppression(self): @@ -358,8 +375,7 @@ class CpplintTest(CpplintTestBase): self.TestLint( 'long a = 65; // NOLINT(readability/casting)', 'Use int16/int64/etc, rather than the C type long' - ' [runtime/int] [4]') - + ' [runtime/int] [4]') # Test Variable Declarations. def testVariableDeclarations(self): @@ -404,6 +420,11 @@ class CpplintTest(CpplintTestBase): self.TestLint('uint a = (uint)NULL;', '') self.TestLint('typedef MockCallback CallbackType;', '') self.TestLint('scoped_ptr< MockCallback > callback_value;', '') + self.TestLint('std::function', '') + self.TestLint('x = sizeof(int)', '') + self.TestLint('x = alignof(int)', '') + self.TestLint('alignas(int) char x[42]', '') + self.TestLint('alignas(alignof(x)) char y[42]', '') # Test taking address of casts (runtime/casting) def testRuntimeCasting(self): @@ -418,6 +439,18 @@ class CpplintTest(CpplintTestBase): 'instead [readability/casting] [4]', error_msg]) + # Alternative error message + alt_error_msg = ('Are you taking an address of something dereferenced ' + 'from a cast? Wrapping the dereferenced expression in ' + 'parentheses will make the binding more obvious' + ' [readability/casting] [4]') + self.TestLint('int* x = &down_cast(obj)->member_;', alt_error_msg) + self.TestLint('int* x = &down_cast(obj)[index];', alt_error_msg) + self.TestLint('int* x = &(down_cast(obj)->member_);', '') + self.TestLint('int* x = &(down_cast(obj)[index]);', '') + self.TestLint('int* x = &down_cast(obj)\n->member_;', alt_error_msg) + self.TestLint('int* x = &(down_cast(obj)\n->member_);', '') + # It's OK to cast an address. self.TestLint('int* x = reinterpret_cast(&foo);', '') @@ -441,13 +474,14 @@ class CpplintTest(CpplintTestBase): def testCheckForUnnamedParams(self): message = ('All parameters should be named in a function' ' [readability/function] [3]') - self.TestLint('virtual void A(int*) const;', message) - self.TestLint('virtual void B(int*);', message) + self.TestLint('virtual void Func(int*) const;', message) + self.TestLint('virtual void Func(int*);', message) self.TestLint('void Method(char*) {', message) self.TestLint('void Method(char*);', message) self.TestLint('static void operator delete[](void*) throw();', message) + self.TestLint('int Method(int);', message) - self.TestLint('virtual void D(int* p);', '') + self.TestLint('virtual void Func(int* p);', '') self.TestLint('void operator delete(void* x) throw();', '') self.TestLint('void Method(char* x) {', '') self.TestLint('void Method(char* /*x*/) {', '') @@ -460,10 +494,13 @@ class CpplintTest(CpplintTestBase): self.TestLint('X operator++(int) {', '') self.TestLint('X operator--(int);', '') self.TestLint('X operator--(int /*unused*/) {', '') + self.TestLint('MACRO(int);', '') self.TestLint('void (*func)(void*);', '') self.TestLint('void Func((*func)(void*)) {}', '') self.TestLint('template void func();', '') + self.TestLint('virtual void f(int /*unused*/) override {', '') + self.TestLint('virtual void f(int /*unused*/) final {', '') # Test deprecated casts such as int(d) def testDeprecatedCast(self): @@ -501,9 +538,34 @@ class CpplintTest(CpplintTestBase): self.TestLint( 'new int64(123); // "new" operator on basic type, weird spacing', '') + self.TestLint( + 'using a = bool(int arg); // C++11 alias-declaration', + '') self.TestLint( 'std::function // C++11 function wrapper', '') + self.TestLint( + 'std::function', + '') + self.TestLint( + 'std::function< int(bool) >', + '') + self.TestLint( + 'mfunction', + '') + self.TestLint( + 'new(field_ptr) int(field->default_value_enum()->number());', + '') + + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData( + 'test.cc', 'cc', + ['// Copyright 2008 Your Company. All Rights Reserved.', + 'typedef std::function<', + ' bool(int)> F;', + ''], + error_collector) + self.assertEquals('', error_collector.Results()) # Return types for function pointers tend to look like casts. self.TestLint( @@ -533,6 +595,9 @@ class CpplintTest(CpplintTestBase): self.TestLint( 'bool TraverseNode(T *Node, bool(VisitorBase:: *traverse) (T *t)) {}', '') + self.TestLint( + 'x = bit_cast(y);', + '') # 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. @@ -546,6 +611,9 @@ class CpplintTest(CpplintTestBase): self.TestLint( 'MOCK_CONST_METHOD2_T(method, double(float, float));', '') + self.TestLint( + 'MOCK_CONST_METHOD1(method, SomeType(int));', + '') error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData('mock.cc', 'cc', @@ -573,7 +641,6 @@ class CpplintTest(CpplintTestBase): 'Use static_cast(...) instead ' '[readability/casting] [4]'))) - # Like gMock method definitions, MockCallback instantiations look very similar # to bad casts. def testMockCallback(self): @@ -835,6 +902,10 @@ class CpplintTest(CpplintTestBase): cpplint.CleanseComments('f(a /* name */, b);')) self.assertEqual('f(a, b);', cpplint.CleanseComments('f(a, /* name */b);')) + self.assertEqual('f(a, b, c);', + cpplint.CleanseComments('f(a, /**/b, /**/c);')) + self.assertEqual('f(a, b, c);', + cpplint.CleanseComments('f(a, /**/b/**/, c);')) def testRawStrings(self): self.TestMultiLineLint( @@ -880,6 +951,26 @@ class CpplintTest(CpplintTestBase): )";""", '') + self.TestMultiLineLint( + """ + void Func() { + string s = StrCat(R"TrueDelimiter( + )" + )FalseDelimiter" + )TrueDelimiter", R"TrueDelimiter2( + )" + )FalseDelimiter2" + )TrueDelimiter2"); + }""", + '') + self.TestMultiLineLint( + """ + static SomeStruct kData = { + {0, R"(line1 + line2 + )"} + };""", + '') def testMultiLineComments(self): # missing explicit is bad @@ -1075,6 +1166,13 @@ class CpplintTest(CpplintTestBase): Foo(const Foo&); };""", '') + # Special case for std::initializer_list + self.TestMultiLineLint( + """ + class Foo { + Foo(std::initializer_list &arg) {} + };""", + '') # Anything goes inside an assembly block error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData('foo.cc', 'cc', @@ -1179,18 +1277,20 @@ class CpplintTest(CpplintTestBase): self.TestLanguageRulesCheck('foo_unittest.cc', '#include ', '') def testCheckPosixThreading(self): - self.TestLint('sctime_r()', '') - self.TestLint('strtok_r()', '') - self.TestLint(' strtok_r(foo, ba, r)', '') - self.TestLint('brand()', '') + self.TestLint('var = sctime_r()', '') + self.TestLint('var = strtok_r()', '') + self.TestLint('var = strtok_r(foo, ba, r)', '') + self.TestLint('var = brand()', '') self.TestLint('_rand()', '') self.TestLint('.rand()', '') self.TestLint('->rand()', '') - self.TestLint('rand()', + self.TestLint('ACMRandom rand(seed)', '') + self.TestLint('ISAACRandom rand()', '') + self.TestLint('var = rand()', 'Consider using rand_r(...) instead of rand(...)' ' for improved thread safety.' ' [runtime/threadsafe_fn] [2]') - self.TestLint('strtok()', + self.TestLint('var = strtok(str, delim)', 'Consider using strtok_r(...) ' 'instead of strtok(...)' ' for improved thread safety.' @@ -1258,9 +1358,7 @@ class CpplintTest(CpplintTestBase): 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 - ]) + ['Extra space after ( [whitespace/parens] [2]', errmsg]) # const string reference members are dangerous.. def testConstStringReferenceMembers(self): @@ -1541,16 +1639,6 @@ class CpplintTest(CpplintTestBase): 'Consider using ASSERT_GE_M instead of ASSERT_FALSE_M(a < b)' ' [readability/check] [2]') - self.TestLint('CHECK(some_iterator == obj.end())', '') - self.TestLint('EXPECT_TRUE(some_iterator == obj.end())', '') - self.TestLint('EXPECT_FALSE(some_iterator == obj.end())', '') - self.TestLint('CHECK(some_pointer != NULL)', '') - self.TestLint('EXPECT_TRUE(some_pointer != NULL)', '') - self.TestLint('EXPECT_FALSE(some_pointer != NULL)', '') - - self.TestLint('CHECK(CreateTestFile(dir, (1 << 20)));', '') - self.TestLint('CHECK(CreateTestFile(dir, (1 >> 20)));', '') - self.TestLint('CHECK(x<42)', ['Missing spaces around <' ' [whitespace/operators] [3]', @@ -1564,11 +1652,6 @@ class CpplintTest(CpplintTestBase): self.TestLint('using some::namespace::operator<<;', '') self.TestLint('using some::namespace::operator>>;', '') - self.TestLint('CHECK(x ^ (y < 42))', '') - self.TestLint('CHECK((x > 42) ^ (x < 54))', '') - self.TestLint('CHECK(a && b < 42)', '') - self.TestLint('CHECK(42 < a && a < b)', '') - self.TestLint('SOFT_CHECK(x > 42)', '') self.TestLint('CHECK(x->y == 42)', 'Consider using CHECK_EQ instead of CHECK(a == b)' @@ -1584,12 +1667,41 @@ class CpplintTest(CpplintTestBase): ' [whitespace/parens] [4]', 'Consider using EXPECT_LT instead of EXPECT_TRUE(a < b)' ' [readability/check] [2]']) - self.TestLint( - 'CHECK("foo" == "foo")', - 'Consider using CHECK_EQ instead of CHECK(a == b)' - ' [readability/check] [2]') - self.TestLint('CHECK_EQ("foo", "foo")', '') + self.TestLint('CHECK(4\'2 == x)', + 'Consider using CHECK_EQ instead of CHECK(a == b)' + ' [readability/check] [2]') + + def testCheckCheckFalsePositives(self): + self.TestLint('CHECK(some_iterator == obj.end())', '') + self.TestLint('EXPECT_TRUE(some_iterator == obj.end())', '') + self.TestLint('EXPECT_FALSE(some_iterator == obj.end())', '') + self.TestLint('CHECK(some_pointer != NULL)', '') + self.TestLint('EXPECT_TRUE(some_pointer != NULL)', '') + self.TestLint('EXPECT_FALSE(some_pointer != NULL)', '') + + self.TestLint('CHECK(CreateTestFile(dir, (1 << 20)));', '') + self.TestLint('CHECK(CreateTestFile(dir, (1 >> 20)));', '') + + self.TestLint('CHECK(x ^ (y < 42))', '') + self.TestLint('CHECK((x > 42) ^ (x < 54))', '') + self.TestLint('CHECK(a && b < 42)', '') + self.TestLint('CHECK(42 < a && a < b)', '') + self.TestLint('SOFT_CHECK(x > 42)', '') + + self.TestMultiLineLint( + """_STLP_DEFINE_BINARY_OP_CHECK(==, _OP_EQUAL); + _STLP_DEFINE_BINARY_OP_CHECK(!=, _OP_NOT_EQUAL); + _STLP_DEFINE_BINARY_OP_CHECK(<, _OP_LESS_THAN); + _STLP_DEFINE_BINARY_OP_CHECK(<=, _OP_LESS_EQUAL); + _STLP_DEFINE_BINARY_OP_CHECK(>, _OP_GREATER_THAN); + _STLP_DEFINE_BINARY_OP_CHECK(>=, _OP_GREATER_EQUAL); + _STLP_DEFINE_BINARY_OP_CHECK(+, _OP_PLUS); + _STLP_DEFINE_BINARY_OP_CHECK(*, _OP_TIMES); + _STLP_DEFINE_BINARY_OP_CHECK(/, _OP_DIVIDE); + _STLP_DEFINE_BINARY_OP_CHECK(-, _OP_SUBTRACT); + _STLP_DEFINE_BINARY_OP_CHECK(%, _OP_MOD);""", + '') # Alternative token to punctuation operator replacements def testCheckAltTokens(self): @@ -1710,7 +1822,7 @@ class CpplintTest(CpplintTestBase): 'vector &nonconst_x) {', operand_error_message % 'vector &nonconst_x') - # Another potential false positive. This one needs full parser + # Other potential false positives. These need full parser # state to reproduce as opposed to just TestLint. error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( @@ -1727,11 +1839,24 @@ class CpplintTest(CpplintTestBase): ' ostream& out', ' const dense_hash_set& seq) {', '}', + '#define UNSUPPORTED_MASK(_mask) \\', + ' if (flags & _mask) { \\', + ' LOG(FATAL) << "Unsupported flag: " << #_mask; \\', + ' }', + 'Constructor::Constructor()', + ' : initializer1_(a & b),', + ' initializer2_(a & b) {', + '}', + 'Constructor::Constructor()', + ' : initializer1_{a & b},', + ' initializer2_(a & b) {', + '}', ''], error_collector) self.assertEquals('', error_collector.Results()) # Multi-line references + error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'foo.cc', 'cc', ['// Copyright 2008 Your Company. All Rights Reserved.', @@ -1756,6 +1881,22 @@ class CpplintTest(CpplintTestBase): operand_error_message % 'Outer::Inner& nonconst_z'], error_collector.Results()) + # A peculiar false positive due to bad template argument parsing + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData( + 'foo.cc', 'cc', + ['// Copyright 2008 Your Company. All Rights Reserved.', + 'inline RCULocked::ReadPtr::ReadPtr(const RCULocked* rcu) {', + ' DCHECK(!(data & kFlagMask)) << "Error";', + '}', + '', + 'RCULocked::WritePtr::WritePtr(RCULocked* rcu)', + ' : lock_(&rcu_->mutex_) {', + '}', + ''], + error_collector.Results()) + self.assertEquals('', error_collector.Results()) + def testBraceAtBeginOfLine(self): self.TestLint('{', '{ should almost always be at the end of the previous line' @@ -1847,6 +1988,9 @@ class CpplintTest(CpplintTestBase): self.TestLint('typedef foo (*foo)(foo)', '') self.TestLint('typedef foo (*foo12bar_)(foo)', '') self.TestLint('typedef foo (Foo::*bar)(foo)', '') + self.TestLint('using foo = type (Foo::*bar)(foo)', '') + self.TestLint('using foo = type (Foo::*bar)(', '') + self.TestLint('using foo = type (Foo::*)(', '') self.TestLint('foo (Foo::*bar)(', '') self.TestLint('foo (x::y::*z)(', '') self.TestLint('foo (Foo::bar)(', @@ -1900,6 +2044,40 @@ class CpplintTest(CpplintTestBase): self.TestLint('file_tocs_[i] = (FileToc) {a, b, c};', '') self.TestMultiLineLint('class X : public Y,\npublic Z {};', '') + def testLambda(self): + self.TestLint('auto x = []() {};', '') + self.TestLint('return []() {};', '') + self.TestMultiLineLint('auto x = []() {\n};\n', '') + self.TestLint('int operator[](int x) {};', + 'You don\'t need a ; after a } [readability/braces] [4]') + + self.TestMultiLineLint('auto x = [&a,\nb]() {};', '') + self.TestMultiLineLint('auto x = [&a,\nb]\n() {};', '') + self.TestMultiLineLint('auto x = [&a,\n' + ' b](\n' + ' int a,\n' + ' int b) {\n' + ' return a +\n' + ' b;\n' + '};\n', + '') + + for lambda_with_default_capture in ('void f() { [=]{}; }', + 'void f() { [=](int i) {}; }', + 'void f() { [=, &x]{}; }', + 'void f() { [&]{}; }', + 'void f() { [ & ]{}; }', + 'void f() { [&, y]{}; }'): + self.TestLint(lambda_with_default_capture, + 'Default lambda captures are an unapproved C++ feature. ' + '[build/c++11] [4]') + + # "[&...]" isn't necessarily a default capture, though "[=...]" always is. + self.TestLint('void f() { [&variable]{}; }', '') + + # Avoid false positives with operator[] + self.TestLint('table_to_children[&*table].push_back(dependent);', '') + def testBraceInitializerList(self): self.TestLint('MyStruct p = {1, 2};', '') self.TestLint('MyStruct p{1, 2};', '') @@ -1919,6 +2097,8 @@ class CpplintTest(CpplintTestBase): self.TestLint('auto foo = std::unique_ptr{new Foo{}};', '') self.TestLint('static_assert(Max7String{}.IsValid(), "");', '') self.TestLint('map_of_pairs[{1, 2}] = 3;', '') + self.TestLint('ItemView{has_offer() ? new Offer{offer()} : nullptr', '') + self.TestLint('template {}> = 0>', '') self.TestMultiLineLint('std::unique_ptr foo{\n' ' new Foo{}\n' @@ -1943,13 +2123,15 @@ class CpplintTest(CpplintTestBase): self.TestLint('} else{', 'Missing space before {' ' [whitespace/braces] [5]') self.TestLint('} else {', '') - self.TestLint('} else if', '') + self.TestLint('} else if (foo) {', '') def testSpacingWithInitializerLists(self): self.TestLint('int v[1][3] = {{1, 2, 3}};', '') self.TestLint('int v[1][1] = {{0}};', '') def testSpacingForBinaryOps(self): + self.TestLint('if (foo||bar) {', 'Missing spaces around ||' + ' [whitespace/operators] [3]') self.TestLint('if (foo<=bar) {', 'Missing spaces around <=' ' [whitespace/operators] [3]') self.TestLint('if (foobar) {', 'Missing spaces around <' ' [whitespace/operators] [3]') self.TestLint('template', '') - self.TestLint('scoped_ptr>', '') + self.TestLint('typedef hash_map', '') self.TestLint('10<<20', '') self.TestLint('10<>* arg1);', '') self.TestLint('foo = new set>;', '') self.TestLint('reinterpret_cast>*>(a);', '') + self.TestLint('MACRO(<<)', '') + self.TestLint('MACRO(<<, arg)', '') + self.TestLint('MACRO(<<=)', '') + self.TestLint('MACRO(<<=, arg)', '') - # These don't warn because they have trailing commas. - self.TestLint('typedef hash_map::operator==;', '') + self.TestLint('using Vector3::operator!=;', '') + + def testRvalueReference(self): + space_error = 'Missing spaces around && [whitespace/operators] [3]' + rvalue_error = ('RValue references are an unapproved C++ feature.' + ' [build/c++11] [3]') + + # Places where lack of space are allowed + self.TestLint('DEFINE_BINARY_OPERATOR(&&)', '') + self.TestLint('bool operator&&(A b) {}', '') + self.TestLint('bool operator&&(A b) {', '') + self.TestLint('bool operator&&(A b);', '') + + # Assignment expressions + self.TestLint('a = b && c;', '') + self.TestLint('a = b&& c;', space_error) + self.TestLint('a = b &&c;', space_error) + self.TestLint('a = (b&& c);', space_error) + self.TestLint('a = (b &&c);', space_error) + self.TestLint('a&& b = c;', rvalue_error) + self.TestLint('a&& c = d;', rvalue_error) + self.TestLint('auto&& a = b;', rvalue_error) + self.TestLint('const a&& b = c;', rvalue_error) + self.TestLint('struct a&& b = c;', rvalue_error) + self.TestLint('decltype(a)&& b = c;', rvalue_error) + + # Cast expressions + self.TestLint('a = const_cast(c);', rvalue_error) + self.TestLint('a = const_cast(c);', rvalue_error) + self.TestLint('a = static_cast(c);', rvalue_error) + self.TestLint('a = static_cast(c);', rvalue_error) + self.TestLint('a = dynamic_cast(c);', rvalue_error) + self.TestLint('a = dynamic_cast(c);', rvalue_error) + self.TestLint('a = reinterpret_cast(c);', rvalue_error) + self.TestLint('a = reinterpret_cast(c);', rvalue_error) + self.TestLint('a = cast < b&& c;', space_error) + + # Function parameters + for head in ['void Func', 'vector Func', 'vector\nFunc', + 'Constructor', 'Constructor::Constructor', + 'operator=', 'operator =', 'operator = ']: + for body in [' {}', ' {', ';']: + self.TestMultiLineLint(head + '(A&& b)' + body, rvalue_error) + self.TestMultiLineLint(head + '(A &&b)' + body, rvalue_error) + self.TestMultiLineLint(head + '(A&&... b)' + body, rvalue_error) + self.TestMultiLineLint(head + '(A&& c)' + body, rvalue_error) + self.TestMultiLineLint(head + '(A &&c)' + body, rvalue_error) + + # Function templates + self.TestLint('std::conditional::type', rvalue_error) + self.TestLint('std::conditional::type', rvalue_error) + + # Template functions + self.TestLint('template R&& F()', rvalue_error) + self.TestLint('template R&& F() {', rvalue_error) + self.TestMultiLineLint('template \nR&& F()', rvalue_error) + self.TestMultiLineLint('template \nR&& F() {', rvalue_error) + + # For loops + self.TestLint('for (a&& b;;)', rvalue_error) + self.TestLint('for (a&& b;;) {', rvalue_error) + self.TestLint('for (; a&& b;)', space_error) + self.TestLint('for (; a&& b;) {', space_error) + + # Constructors + self.TestLint('A(a&& b)', rvalue_error) + self.TestLint('explicit A(a&& b)', rvalue_error) + self.TestLint('A(a b) : c(d&& e)', space_error) + self.TestLint('A(a b) : c(), d(e&& f)', space_error) + + # Verify that RValue reference warnings for a line range can be silenced + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData('foo.cc', 'cc', + ['// Copyright 2008 Your Company.', + 'GOOGLE_ALLOW_RVALUE_REFERENCES_PUSH', + 'void F(A&& b);', + 'GOOGLE_ALLOW_RVALUE_REFERENCES_POP', + ''], + error_collector) + self.assertEquals(error_collector.Results(), '') def testSpacingBeforeLastSemicolon(self): self.TestLint('call_function() ;', @@ -2090,6 +2354,22 @@ class CpplintTest(CpplintTestBase): 'For a static/global string constant, use a C style ' 'string instead: "char Foo::bar[]".' ' [runtime/string] [4]') + self.TestLint('string* pointer', '') + self.TestLint('string *pointer', '') + self.TestLint('string* pointer = Func();', '') + self.TestLint('string *pointer = Func();', '') + self.TestLint('const string* pointer', '') + self.TestLint('const string *pointer', '') + self.TestLint('const string* pointer = Func();', '') + self.TestLint('const string *pointer = Func();', '') + self.TestLint('string const* pointer', '') + self.TestLint('string const *pointer', '') + self.TestLint('string const* pointer = Func();', '') + self.TestLint('string const *pointer = Func();', '') + self.TestLint('string* const pointer', '') + self.TestLint('string *const pointer', '') + self.TestLint('string* const pointer = Func();', '') + self.TestLint('string *const pointer = Func();', '') self.TestLint('string Foo::bar() {}', '') self.TestLint('string Foo::operator*() {}', '') # Rare case. @@ -2183,10 +2463,23 @@ class CpplintTest(CpplintTestBase): self.TestLint('int i = 0; // Having three spaces is OK.', '') self.TestLint('// Top level comment', '') self.TestLint(' // Line starts with two spaces.', '') - self.TestLint('foo();\n' - '{ // A scope is opening.', '') - self.TestLint(' foo();\n' - ' { // An indented scope is opening.', '') + self.TestMultiLineLint('void foo() {\n' + ' { // A scope is opening.\n' + ' int a;', '') + self.TestMultiLineLint('void foo() {\n' + ' { // A scope is opening.\n' + '#define A a', + 'At least two spaces is best between code and ' + 'comments [whitespace/comments] [2]') + self.TestMultiLineLint(' foo();\n' + ' { // An indented scope is opening.\n' + ' int a;', '') + self.TestMultiLineLint('vector my_elements = {// first\n' + ' 1,', '') + self.TestMultiLineLint('vector my_elements = {// my_elements is ..\n' + ' 1,', + 'At least two spaces is best between code and ' + 'comments [whitespace/comments] [2]') self.TestLint('if (foo) { // not a pure scope; comment is too close!', 'At least two spaces is best between code and comments' ' [whitespace/comments] [2]') @@ -2209,14 +2502,13 @@ class CpplintTest(CpplintTestBase): self.TestLint('///', '') # Empty Doxygen comment self.TestLint('////x', 'Should have a space between // and comment' ' [whitespace/comments] [4]') + self.TestLint('//}', '') + self.TestLint('//}x', 'Should have a space between // and comment' + ' [whitespace/comments] [4]') self.TestLint('//! + static void Func() { + }""", + '') + + def testConditionals(self): + self.TestMultiLineLint( + """ + if (foo) + goto fail; + goto fail;""", + 'If/else bodies with multiple statements require braces' + ' [readability/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) + goto fail; goto fail;""", + 'If/else bodies with multiple statements require braces' + ' [readability/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) + foo; + else + goto fail; + goto fail;""", + 'If/else bodies with multiple statements require braces' + ' [readability/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) goto fail; + goto fail;""", + 'If/else bodies with multiple statements require braces' + ' [readability/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) + if (bar) + baz; + else + qux;""", + 'Else clause should be indented at the same level as if. Ambiguous' + ' nested if/else chains require braces. [readability/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) + if (bar) + baz; + else + qux;""", + 'Else clause should be indented at the same level as if. Ambiguous' + ' nested if/else chains require braces. [readability/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) { + bar; + baz; + } else + qux;""", + 'If an else has a brace on one side, it should have it on both' + ' [readability/braces] [5]') + self.TestMultiLineLint( + """ + if (foo) + bar; + else { + baz; + }""", + 'If an else has a brace on one side, it should have it on both' + ' [readability/braces] [5]') + self.TestMultiLineLint( + """ + if (foo) + bar; + else if (baz) { + qux; + }""", + 'If an else has a brace on one side, it should have it on both' + ' [readability/braces] [5]') + self.TestMultiLineLint( + """ + if (foo) { + bar; + } else if (baz) + qux;""", + 'If an else has a brace on one side, it should have it on both' + ' [readability/braces] [5]') + self.TestMultiLineLint( + """ + if (foo) + goto fail; + bar;""", + '') + self.TestMultiLineLint( + """ + if (foo + && bar) { + baz; + qux; + }""", + '') + self.TestMultiLineLint( + """ + if (foo) + goto + fail;""", + '') + self.TestMultiLineLint( + """ + if (foo) + bar; + else + baz; + qux;""", + '') + self.TestMultiLineLint( + """ + for (;;) { + if (foo) + bar; + else + baz; + }""", + '') + self.TestMultiLineLint( + """ + if (foo) + bar; + else if (baz) + baz;""", + '') + self.TestMultiLineLint( + """ + if (foo) + bar; + else + baz;""", + '') + self.TestMultiLineLint( + """ + if (foo) { + bar; + } else { + baz; + }""", + '') + self.TestMultiLineLint( + """ + if (foo) { + bar; + } else if (baz) { + qux; + }""", + '') + # Note: this is an error for a different reason, but should not trigger the + # single-line if error. + self.TestMultiLineLint( + """ + if (foo) + { + bar; + baz; + }""", + '{ should almost always be at the end of the previous line' + ' [whitespace/braces] [4]') + self.TestMultiLineLint( + """ + if (foo) { \\ + bar; \\ + baz; \\ + }""", + '') + self.TestMultiLineLint( + """ + void foo() { if (bar) baz; }""", + '') + self.TestMultiLineLint( + """ + #if foo + bar; + #else + baz; + qux; + #endif""", + '') def testTab(self): self.TestLint('\tint a;', @@ -3038,29 +3603,30 @@ class CpplintTest(CpplintTestBase): ' [build/include] [4]') def testBuildPrintfFormat(self): - self.TestLint( - r'printf("\%%d", value);', - '%, [, (, and { are undefined character escapes. Unescape them.' - ' [build/printf_format] [3]') + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData( + 'foo.cc', 'cc', + [r'printf("\%%d", value);', + r'snprintf(buffer, sizeof(buffer), "\[%d", value);', + r'fprintf(file, "\(%d", value);', + r'vsnprintf(buffer, sizeof(buffer), "\\\{%d", ap);'], + error_collector) + self.assertEquals( + 4, + error_collector.Results().count( + '%, [, (, and { are undefined character escapes. Unescape them.' + ' [build/printf_format] [3]')) - self.TestLint( - r'snprintf(buffer, sizeof(buffer), "\[%d", value);', - '%, [, (, and { are undefined character escapes. Unescape them.' - ' [build/printf_format] [3]') - - self.TestLint( - r'fprintf(file, "\(%d", value);', - '%, [, (, and { are undefined character escapes. Unescape them.' - ' [build/printf_format] [3]') - - self.TestLint( - r'vsnprintf(buffer, sizeof(buffer), "\\\{%d", ap);', - '%, [, (, and { are undefined character escapes. Unescape them.' - ' [build/printf_format] [3]') - - # Don't warn if double-slash precedes the symbol - self.TestLint(r'printf("\\%%%d", value);', - '') + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData( + 'foo.cc', 'cc', + ['// Copyright 2008 Your Company.', + r'printf("\\%%%d", value);', + r'printf(R"(\[)");', + r'printf(R"(\[%s)", R"(\])");', + ''], + error_collector) + self.assertEquals('', error_collector.Results()) def testRuntimePrintfFormat(self): self.TestLint( @@ -3211,6 +3777,65 @@ class CpplintTest(CpplintTestBase): self.TestLint('snprintf(fisk, 1, format)', 'If you can, use sizeof(fisk) instead of 1 as the 2nd arg ' 'to snprintf. [runtime/printf] [3]') +class Cxx11Test(CpplintTestBase): + + def Helper(self, package, extension, lines, count): + filename = package + '/foo.' + extension + lines = lines[:] + + # Header files need to have an ifdef guard wrapped around their code. + if extension == 'h': + guard = filename.upper().replace('/', '_').replace('.', '_') + '_' + lines.insert(0, '#ifndef ' + guard) + lines.insert(1, '#define ' + guard) + lines.append('#endif // ' + guard) + + # All files need a final blank line. + lines.append('') + + # Process the file and check resulting error count. + collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData(filename, extension, lines, collector) + error_list = collector.ResultList() + self.assertEquals(count, len(error_list), error_list) + + def TestCxx11Feature(self, code, expected_error): + lines = code.split('\n') + collector = ErrorCollector(self.assert_) + cpplint.RemoveMultiLineComments('foo.h', lines, collector) + clean_lines = cpplint.CleansedLines(lines) + cpplint.FlagCxx11Features('foo.cc', clean_lines, 0, collector) + self.assertEquals(expected_error, collector.Results()) + + def testBlockedHeaders(self): + self.TestCxx11Feature('#include ', + ' is an unapproved C++11 header.' + ' [build/c++11] [5]') + + def testBlockedClasses(self): + self.TestCxx11Feature('std::alignment_of', + 'std::alignment_of is an unapproved ' + 'C++11 class or function. Send c-style an example ' + 'of where it would make your code more readable, ' + 'and they may let you use it.' + ' [build/c++11] [5]') + self.TestCxx11Feature('std::alignment_offer', '') + self.TestCxx11Feature('mystd::alignment_of', '') + self.TestCxx11Feature('std::binomial_distribution', '') + + def testBlockedFunctions(self): + self.TestCxx11Feature('std::alignment_of', + 'std::alignment_of is an unapproved ' + 'C++11 class or function. Send c-style an example ' + 'of where it would make your code more readable, ' + 'and they may let you use it.' + ' [build/c++11] [5]') + # Missed because of the lack of "std::". Compiles because ADL + # looks in the namespace of my_shared_ptr, which (presumably) is + # std::. But there will be a lint error somewhere in this file + # since my_shared_ptr had to be defined. + self.TestCxx11Feature('static_pointer_cast(my_shared_ptr)', '') + self.TestCxx11Feature('std::declval()', '') def testExplicitMakePair(self): self.TestLint('make_pair', '') @@ -3228,6 +3853,7 @@ class CpplintTest(CpplintTestBase): self.TestLint('my_make_pair', '') class CleansedLinesTest(unittest.TestCase): + def testInit(self): lines = ['Line 1', 'Line 2', @@ -3278,7 +3904,22 @@ class CleansedLinesTest(unittest.TestCase): self.assertEquals('', collapse('\\012')) # '\012' (char) self.assertEquals('', collapse('\\xfF0')) # '\xfF0' (char) self.assertEquals('', collapse('\\n')) # '\n' (char) - self.assertEquals('\#', collapse('\\#')) # '\#' (bad) + self.assertEquals(r'\#', collapse('\\#')) # '\#' (bad) + + self.assertEquals('"" + ""', collapse('"\'" + "\'"')) + self.assertEquals("'', ''", collapse("'\"', '\"'")) + self.assertEquals('""[0b10]', collapse('"a\'b"[0b1\'0]')) + + self.assertEquals('42', collapse("4'2")) + self.assertEquals('0b0101', collapse("0b0'1'0'1")) + self.assertEquals('1048576', collapse("1'048'576")) + self.assertEquals('0X100000', collapse("0X10'0000")) + self.assertEquals('0004000000', collapse("0'004'000'000")) + self.assertEquals('1.602176565e-19', collapse("1.602'176'565e-19")) + self.assertEquals('\'\' + 0xffff', collapse("'i' + 0xf'f'f'f")) + self.assertEquals('sizeof\'\' == 1', collapse("sizeof'x' == 1")) + self.assertEquals('0x.03p100', collapse('0x.0\'3p1\'0\'0')) + self.assertEquals('123.45', collapse('1\'23.4\'5')) self.assertEquals('StringReplace(body, "", "");', collapse('StringReplace(body, "\\\\", "\\\\\\\\");')) @@ -3287,6 +3928,7 @@ class CleansedLinesTest(unittest.TestCase): class OrderOfIncludesTest(CpplintTestBase): + def setUp(self): self.include_state = cpplint._IncludeState() # Cheat os.path.abspath called in FileInfo class. @@ -3408,6 +4050,7 @@ class OrderOfIncludesTest(CpplintTestBase): '"foo/foo-inl.h"', '', '', + '', '"bar/bar-inl.h"', '"bar/bar.h"']), '') @@ -3513,6 +4156,7 @@ class OrderOfIncludesTest(CpplintTestBase): class CheckForFunctionLengthsTest(CpplintTestBase): + def setUp(self): # Reducing these thresholds for the tests speeds up tests significantly. self.old_normal_trigger = cpplint._FunctionState._NORMAL_TRIGGER @@ -3822,9 +4466,107 @@ class CheckForFunctionLengthsTest(CpplintTestBase): 'Lint failed to find start of function body.' ' [readability/fn_size] [5]') -class NestnigStateTest(unittest.TestCase): + +def TrimExtraIndent(text_block): + """Trim a uniform amount of whitespace off of each line in a string. + + Compute the minimum indent on all non blank lines and trim that from each, so + that the block of text has no extra indentation. + + Args: + text_block: a multiline string + + Returns: + text_block with the common whitespace indent of each line removed. + """ + + def CountLeadingWhitespace(s): + count = 0 + for c in s: + if not c.isspace(): + break + count += 1 + return count + # find the minimum indent (except for blank lines) + min_indent = min([CountLeadingWhitespace(line) + for line in text_block.split('\n') if line]) + return '\n'.join([line[min_indent:] for line in text_block.split('\n')]) + + +class CloseExpressionTest(unittest.TestCase): + def setUp(self): - self.nesting_state = cpplint._NestingState() + self.lines = cpplint.CleansedLines( + # 1 2 3 4 5 + # 0123456789012345678901234567890123456789012345678901234567890 + ['// Line 0', + 'inline RCULocked::ReadPtr::ReadPtr(const RCULocked* rcu) {', + ' DCHECK(!(data & kFlagMask)) << "Error";', + '}', + '// Line 4', + 'RCULocked::WritePtr::WritePtr(RCULocked* rcu)', + ' : lock_(&rcu_->mutex_) {', + '}', + '// Line 8', + 'template ', + 'typename std::enable_if<', + ' std::is_array::value && (std::extent::value > 0)>::type', + 'MakeUnique(A&&... a) = delete;', + '// Line 13', + 'auto x = []() {};', + '// Line 15', + 'template ', + 'friend bool operator==(const reffed_ptr& a,', + ' const reffed_ptr& b) {', + ' return a.get() == b.get();', + '}', + '// Line 21']) + + def testCloseExpression(self): + # List of positions to test: + # (start line, start position, end line, end position + 1) + positions = [(1, 16, 1, 19), + (1, 37, 1, 59), + (1, 60, 3, 1), + (2, 8, 2, 29), + (2, 30, 22, -1), # Left shift operator + (9, 9, 9, 36), + (10, 23, 11, 59), + (11, 54, 22, -1), # Greater than operator + (14, 9, 14, 11), + (14, 11, 14, 13), + (14, 14, 14, 16), + (17, 22, 18, 46), + (18, 47, 20, 1)] + for p in positions: + (_, line, column) = cpplint.CloseExpression(self.lines, p[0], p[1]) + self.assertEquals((p[2], p[3]), (line, column)) + + def testReverseCloseExpression(self): + # List of positions to test: + # (end line, end position, start line, start position) + positions = [(1, 18, 1, 16), + (1, 58, 1, 37), + (2, 27, 2, 10), + (2, 28, 2, 8), + (6, 18, 0, -1), # -> operator + (9, 35, 9, 9), + (11, 54, 0, -1), # Greater than operator + (11, 57, 11, 31), + (14, 10, 14, 9), + (14, 12, 14, 11), + (14, 15, 14, 14), + (18, 45, 17, 22), + (20, 0, 18, 47)] + for p in positions: + (_, line, column) = cpplint.ReverseCloseExpression(self.lines, p[0], p[1]) + self.assertEquals((p[2], p[3]), (line, column)) + + +class NestingStateTest(unittest.TestCase): + + def setUp(self): + self.nesting_state = cpplint.NestingState() self.error_collector = ErrorCollector(self.assert_) def UpdateWithLines(self, lines): @@ -4035,6 +4777,23 @@ class NestnigStateTest(unittest.TestCase): self.assertTrue(isinstance(self.nesting_state.stack[0], cpplint._ClassInfo)) self.assertEquals(self.nesting_state.stack[0].name, 'D') + self.UpdateWithLines(['{', '};']) + self.assertEquals(len(self.nesting_state.stack), 0) + + self.UpdateWithLines(['template ', + 'static void Func() {']) + self.assertEquals(len(self.nesting_state.stack), 1) + self.assertFalse(isinstance(self.nesting_state.stack[0], + cpplint._ClassInfo)) + self.UpdateWithLines(['}', + 'template class K {']) + self.assertEquals(len(self.nesting_state.stack), 1) + self.assertTrue(isinstance(self.nesting_state.stack[0], cpplint._ClassInfo)) + self.assertEquals(self.nesting_state.stack[0].name, 'K') + def testTemplateInnerClass(self): self.UpdateWithLines(['class A {', ' public:'])