diff --git a/cpplint/cpplint.py b/cpplint/cpplint.py index fca5d6a..e60e940 100755 --- a/cpplint/cpplint.py +++ b/cpplint/cpplint.py @@ -194,6 +194,7 @@ _ERROR_CATEGORIES = [ 'readability/constructors', 'readability/fn_size', 'readability/function', + 'readability/inheritance', 'readability/multiline_comment', 'readability/multiline_string', 'readability/namespace', @@ -210,6 +211,7 @@ _ERROR_CATEGORIES = [ 'runtime/invalid_increment', 'runtime/member_string_references', 'runtime/memset', + 'runtime/indentation_namespace', 'runtime/operator', 'runtime/printf', 'runtime/printf_format', @@ -383,6 +385,15 @@ _CPP_HEADERS = frozenset([ ]) +# These headers are excluded from [build/include] and [build/include_order] +# checks: +# - Anything not following google file name conventions (containing an +# uppercase character, such as Python.h or nsStringAPI.h, for example). +# - Lua headers. +_THIRD_PARTY_HEADERS_PATTERN = re.compile( + r'^(?:[^/]*[A-Z][^/]*\.h|lua\.h|lauxlib\.h|lualib\.h)$') + + # Assertion macros. These are defined in base/logging.h and # testing/base/gunit.h. Note that the _M versions need to come first # for substring matching to work. @@ -465,9 +476,6 @@ _MATCH_ASM = re.compile(r'^\s*(?:asm|_asm|__asm|__asm__)' _regexp_compile_cache = {} -# Finds occurrences of NOLINT or NOLINT(...). -_RE_SUPPRESSION = re.compile(r'\bNOLINT\b(\([^)]*\))?') - # {str, set(int)}: a map from error categories to sets of linenumbers # on which those errors are expected and should be suppressed. _error_suppressions = {} @@ -497,24 +505,27 @@ def ParseNolintSuppressions(filename, raw_line, linenum, error): linenum: int, the number of the current line. error: function, an error handler. """ - # FIXME(adonovan): "NOLINT(" is misparsed as NOLINT(*). - matched = _RE_SUPPRESSION.search(raw_line) + matched = Search(r'\bNOLINT(NEXTLINE)?\b(\([^)]+\))?', raw_line) if matched: - category = matched.group(1) + if matched.group(1): + suppressed_line = linenum + 1 + else: + suppressed_line = linenum + category = matched.group(2) if category in (None, '(*)'): # => "suppress all" - _error_suppressions.setdefault(None, set()).add(linenum) + _error_suppressions.setdefault(None, set()).add(suppressed_line) else: if category.startswith('(') and category.endswith(')'): category = category[1:-1] if category in _ERROR_CATEGORIES: - _error_suppressions.setdefault(category, set()).add(linenum) + _error_suppressions.setdefault(category, set()).add(suppressed_line) else: error(filename, linenum, 'readability/nolint', 5, 'Unknown NOLINT error category: %s' % category) def ResetNolintSuppressions(): - "Resets the set of NOLINT suppressions to empty." + """Resets the set of NOLINT suppressions to empty.""" _error_suppressions.clear() @@ -569,11 +580,12 @@ def Search(pattern, s): return _regexp_compile_cache[pattern].search(s) -class _IncludeState(dict): +class _IncludeState(object): """Tracks line numbers for includes, and the order in which includes appear. - As a dict, an _IncludeState object serves as a mapping between include - filename and line number on which that file was included. + include_list contains list of lists of (header, line number) pairs. + It's a lists of lists rather than just one flat list to make it + easier to update across preprocessor boundaries. Call CheckNextIncludeOrder() once for each header in the file, passing in the type constants defined above. Calls in an illegal order will @@ -604,15 +616,42 @@ class _IncludeState(dict): } def __init__(self): - dict.__init__(self) - self.ResetSection() + self.include_list = [[]] + self.ResetSection('') - def ResetSection(self): + def FindHeader(self, header): + """Check if a header has already been included. + + Args: + header: header to check. + Returns: + Line number of previous occurrence, or -1 if the header has not + been seen before. + """ + for section_list in self.include_list: + for f in section_list: + if f[0] == header: + return f[1] + return -1 + + def ResetSection(self, directive): + """Reset section checking for preprocessor directive. + + Args: + directive: preprocessor directive (e.g. "if", "else"). + """ # The name of the current section. self._section = self._INITIAL_SECTION # The path of last found header. self._last_header = '' + # Update list of includes. Note that we never pop from the + # include list. + if directive in ('if', 'ifdef', 'ifndef'): + self.include_list.append([]) + elif directive in ('else', 'elif'): + self.include_list[-1] = [] + def SetLastHeader(self, header_path): self._last_header = header_path @@ -923,7 +962,7 @@ class _IncludeError(Exception): pass -class FileInfo: +class FileInfo(object): """Provides utility functions for filenames. FileInfo provides easy access to the components of a file's path @@ -1634,6 +1673,16 @@ def CheckForHeaderGuard(filename, lines, error): error: The function to call with any errors found. """ + # Don't check for header guards if there are error suppression + # comments somewhere in this file. + # + # Because this is silencing a warning for a nonexistent line, we + # only support the very specific NOLINT(build/header_guard) syntax, + # and not the general NOLINT or NOLINT(*) syntax. + for i in lines: + if Search(r'//\s*NOLINT\(build/header_guard\)', i): + return + cppvar = GetHeaderGuardCPPVariable(filename) ifndef = None @@ -1880,6 +1929,20 @@ def CheckInvalidIncrement(filename, clean_lines, linenum, error): 'Changing pointer instead of value (or unused value of operator*).') +def IsMacroDefinition(clean_lines, linenum): + if Search(r'^#define', clean_lines[linenum]): + return True + + if linenum > 0 and Search(r'\\$', clean_lines[linenum - 1]): + return True + + return False + + +def IsForwardClassDeclaration(clean_lines, linenum): + return Match(r'^\s*(\btemplate\b)*.*class\s+\w+;\s*$', clean_lines[linenum]) + + class _BlockInfo(object): """Stores information about a generic block of code.""" @@ -1887,6 +1950,7 @@ class _BlockInfo(object): self.seen_open_brace = seen_open_brace self.open_parentheses = 0 self.inline_asm = _NO_ASM + self.check_namespace_indentation = False def CheckBegin(self, filename, clean_lines, linenum, error): """Run checks that applies to text up to the opening brace. @@ -1943,6 +2007,7 @@ class _ClassInfo(_BlockInfo): self.name = name self.starting_linenum = linenum self.is_derived = False + self.check_namespace_indentation = True if class_or_struct == 'struct': self.access = 'public' self.is_struct = True @@ -1994,6 +2059,7 @@ class _NamespaceInfo(_BlockInfo): _BlockInfo.__init__(self, False) self.name = name or '' self.starting_linenum = linenum + self.check_namespace_indentation = True def CheckEnd(self, filename, clean_lines, linenum, error): """Check end of namespace comments.""" @@ -2531,17 +2597,73 @@ def CheckForNonStandardConstructs(filename, clean_lines, linenum, base_classname = classinfo.name.split('::')[-1] # Look for single-argument constructors that aren't marked explicit. - # Technically a valid construct, but against style. - args = Match(r'\s+(?:inline\s+)?%s\s*\(([^,()]+)\)' - % re.escape(base_classname), - line) - if (args and - args.group(1) != 'void' and - not Search(r'\bstd::initializer_list\b', args.group(1)) and - not Match(r'(const\s+)?%s(\s+const)?\s*(?:<\w+>\s*)?&' - % re.escape(base_classname), args.group(1).strip())): - error(filename, linenum, 'runtime/explicit', 5, - 'Single-argument constructors should be marked explicit.') + # Technically a valid construct, but against style. Also look for + # non-single-argument constructors which are also technically valid, but + # strongly suggest something is wrong. + explicit_constructor_match = Match( + r'\s+(?:inline\s+)?(explicit\s+)?(?:inline\s+)?%s\s*' + r'\(((?:[^()]|\([^()]*\))*)\)' + % re.escape(base_classname), + line) + + if explicit_constructor_match: + is_marked_explicit = explicit_constructor_match.group(1) + + if not explicit_constructor_match.group(2): + constructor_args = [] + else: + constructor_args = explicit_constructor_match.group(2).split(',') + + # collapse arguments so that commas in template parameter lists and function + # argument parameter lists don't split arguments in two + i = 0 + while i < len(constructor_args): + constructor_arg = constructor_args[i] + while (constructor_arg.count('<') > constructor_arg.count('>') or + constructor_arg.count('(') > constructor_arg.count(')')): + constructor_arg += ',' + constructor_args[i + 1] + del constructor_args[i + 1] + constructor_args[i] = constructor_arg + i += 1 + + defaulted_args = [arg for arg in constructor_args if '=' in arg] + noarg_constructor = (not constructor_args or # empty arg list + # 'void' arg specifier + (len(constructor_args) == 1 and + constructor_args[0].strip() == 'void')) + onearg_constructor = ((len(constructor_args) == 1 and # exactly one arg + not noarg_constructor) or + # all but at most one arg defaulted + (len(constructor_args) >= 1 and + not noarg_constructor and + len(defaulted_args) >= len(constructor_args) - 1)) + initializer_list_constructor = bool( + onearg_constructor and + Search(r'\bstd\s*::\s*initializer_list\b', constructor_args[0])) + copy_constructor = bool( + onearg_constructor and + Match(r'(const\s+)?%s(\s*<[^>]*>)?(\s+const)?\s*(?:<\w+>\s*)?&' + % re.escape(base_classname), constructor_args[0].strip())) + + if (not is_marked_explicit and + onearg_constructor and + not initializer_list_constructor and + not copy_constructor): + if defaulted_args: + error(filename, linenum, 'runtime/explicit', 5, + 'Constructors callable with one argument ' + 'should be marked explicit.') + else: + error(filename, linenum, 'runtime/explicit', 5, + 'Single-parameter constructors should be marked explicit.') + elif is_marked_explicit and not onearg_constructor: + if noarg_constructor: + error(filename, linenum, 'runtime/explicit', 5, + 'Zero-parameter constructors should not be marked explicit.') + else: + error(filename, linenum, 'runtime/explicit', 0, + 'Constructors that require multiple arguments ' + 'should not be marked explicit.') def CheckSpacingForFunctionCall(filename, clean_lines, linenum, error): @@ -2634,6 +2756,20 @@ def IsBlankLine(line): return not line or line.isspace() +def CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, + error): + is_namespace_indent_item = ( + len(nesting_state.stack) > 1 and + nesting_state.stack[-1].check_namespace_indentation and + isinstance(nesting_state.previous_stack_top, _NamespaceInfo) and + nesting_state.previous_stack_top == nesting_state.stack[-2]) + + if ShouldCheckNamespaceIndentation(nesting_state, is_namespace_indent_item, + clean_lines.elided, line): + CheckItemIndentationInNamespace(filename, clean_lines.elided, + line, error) + + def CheckForFunctionLengths(filename, clean_lines, linenum, function_state, error): """Reports for long function bodies. @@ -2772,7 +2908,6 @@ def CheckAccess(filename, clean_lines, linenum, nesting_state, error): line = clean_lines.elided[linenum] # get rid of comments and strings matched = Match((r'\s*(DISALLOW_COPY_AND_ASSIGN|' - r'DISALLOW_EVIL_CONSTRUCTORS|' r'DISALLOW_IMPLICIT_CONSTRUCTORS)'), line) if not matched: return @@ -2994,6 +3129,7 @@ def CheckOperatorSpacing(filename, clean_lines, linenum, error): # We allow no-spaces around << when used like this: 10<<20, but # not otherwise (particularly, not when used as streams) + # # We also allow operators following an opening parenthesis, since # those tend to be macros that deal with operators. match = Search(r'(operator|\S)(?:L|UL|ULL|l|ul|ull)?<<([^\s,=])', line) @@ -3087,7 +3223,8 @@ def CheckCommaSpacing(filename, clean_lines, linenum, error): # verify that lines contain missing whitespaces, second pass on raw # lines to confirm that those missing whitespaces are not due to # elided comments. - if Search(r',[^,\s]', line) and Search(r',[^,\s]', raw[linenum]): + if (Search(r',[^,\s]', ReplaceAll(r'\boperator\s*,\s*\(', 'F(', line)) and + Search(r',[^,\s]', raw[linenum])): error(filename, linenum, 'whitespace/comma', 3, 'Missing space after ,') @@ -3392,7 +3529,7 @@ def IsRValueType(clean_lines, nesting_state, linenum, column): match_symbol.group(1)) if match_func: # Check for constructors, which don't have return types. - if Search(r'\bexplicit$', match_func.group(1)): + if Search(r'\b(?:explicit|inline)$', match_func.group(1)): return True implicit_constructor = Match(r'\s*(\w+)\((?:const\s+)?(\w+)', prefix) if (implicit_constructor and @@ -3417,8 +3554,27 @@ def IsRValueType(clean_lines, nesting_state, linenum, column): return False +def IsDeletedOrDefault(clean_lines, linenum): + """Check if current constructor or operator is deleted or default. + + Args: + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + Returns: + True if this is a deleted or default constructor. + """ + open_paren = clean_lines.elided[linenum].find('(') + if open_paren < 0: + return False + (close_line, _, close_paren) = CloseExpression( + clean_lines, linenum, open_paren) + if close_paren < 0: + return False + return Match(r'\s*=\s*(?:delete|default)\b', close_line[close_paren:]) + + def IsRValueAllowed(clean_lines, linenum): - """Check if RValue reference is allowed within some range of lines. + """Check if RValue reference is allowed on a particular line. Args: clean_lines: A CleansedLines instance containing the file. @@ -3426,6 +3582,7 @@ def IsRValueAllowed(clean_lines, linenum): Returns: True if line is within the region where RValue references are allowed. """ + # Allow region marked by PUSH/POP macros for i in xrange(linenum, 0, -1): line = clean_lines.elided[i] if Match(r'GOOGLE_ALLOW_RVALUE_REFERENCES_(?:PUSH|POP)', line): @@ -3435,6 +3592,26 @@ def IsRValueAllowed(clean_lines, linenum): line = clean_lines.elided[j] if Match(r'GOOGLE_ALLOW_RVALUE_REFERENCES_(?:PUSH|POP)', line): return line.endswith('POP') + + # Allow operator= + line = clean_lines.elided[linenum] + if Search(r'\boperator\s*=\s*\(', line): + return IsDeletedOrDefault(clean_lines, linenum) + + # Allow constructors + match = Match(r'\s*([\w<>]+)\s*::\s*([\w<>]+)\s*\(', line) + if match and match.group(1) == match.group(2): + return IsDeletedOrDefault(clean_lines, linenum) + if Search(r'\b(?:explicit|inline)\s+[\w<>]+\s*\(', line): + return IsDeletedOrDefault(clean_lines, linenum) + + if Match(r'\s*[\w<>]+\s*\(', line): + previous_line = 'ReturnType' + if linenum > 0: + previous_line = clean_lines.elided[linenum - 1] + if Match(r'^\s*$', previous_line) or Search(r'[{}:;]\s*$', previous_line): + return IsDeletedOrDefault(clean_lines, linenum) + return False @@ -3643,9 +3820,13 @@ def CheckBraces(filename, clean_lines, linenum, error): # methods) and a single \ after the semicolon (for macros) endpos = endline.find(';') if not Match(r';[\s}]*(\\?)$', endline[endpos:]): - # Semicolon isn't the last character, there's something trailing - error(filename, linenum, 'readability/braces', 4, - 'If/else bodies with multiple statements require braces') + # Semicolon isn't the last character, there's something trailing. + # Output a warning if the semicolon is not contained inside + # a lambda expression. + if not Match(r'^[^{};]*\[[^\[\]]*\][^{}]*\{[^{}]*\}\s*\)*[;,]\s*$', + endline): + error(filename, linenum, 'readability/braces', 4, + 'If/else bodies with multiple statements require braces') elif endlinenum < len(clean_lines.elided) - 1: # Make sure the next line is dedented next_line = clean_lines.elided[endlinenum + 1] @@ -3876,6 +4057,13 @@ def CheckCheck(filename, clean_lines, linenum, error): clean_lines, linenum, start_pos) if end_pos < 0: return + + # If the check macro is followed by something other than a + # semicolon, assume users will log their own custom error messages + # and don't suggest any replacements. + if not Match(r'\s*;', last_line[end_pos:]): + return + if linenum == end_line: expression = lines[linenum][start_pos + 1:end_pos - 1] else: @@ -4139,7 +4327,6 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, CheckSectionSpacing(filename, clean_lines, classinfo, linenum, error) -_RE_PATTERN_INCLUDE_NEW_STYLE = re.compile(r'#include +"[^/]+\.h"') _RE_PATTERN_INCLUDE = re.compile(r'^\s*#\s*include\s*([<"])([^>"]*)[>"].*$') # Matches the first component of a filename delimited by -s and _s. That is: # _RE_FIRST_COMPONENT.match('foo').group(0) == 'foo' @@ -4271,7 +4458,14 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): line = clean_lines.lines[linenum] # "include" should use the new style "foo/bar.h" instead of just "bar.h" - if _RE_PATTERN_INCLUDE_NEW_STYLE.search(line): + # Only do this check if the included header follows google naming + # conventions. If not, assume that it's a 3rd party API that + # requires special include conventions. + # + # We also make an exception for Lua headers, which follow google + # naming convention but not the include convention. + match = Match(r'#include\s*"([^/]+\.h)"', line) + if match and not _THIRD_PARTY_HEADERS_PATTERN.match(match.group(1)): error(filename, linenum, 'build/include', 4, 'Include the directory when naming .h files') @@ -4282,12 +4476,13 @@ def CheckIncludeLine(filename, clean_lines, linenum, include_state, error): if match: include = match.group(2) is_system = (match.group(1) == '<') - if include in include_state: + duplicate_line = include_state.FindHeader(include) + if duplicate_line >= 0: error(filename, linenum, 'build/include', 4, '"%s" already included at %s:%s' % - (include, filename, include_state[include])) - else: - include_state[include] = linenum + (include, filename, duplicate_line)) + elif not _THIRD_PARTY_HEADERS_PATTERN.match(include): + include_state.include_list[-1].append((include, linenum)) # We want to ensure that headers appear in the right order: # 1) for foo.cc, foo.h (preferred location) @@ -4441,8 +4636,9 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, # Reset include state across preprocessor directives. This is meant # to silence warnings for conditional includes. - if Match(r'^\s*#\s*(?:ifdef|elif|else|endif)\b', line): - include_state.ResetSection() + match = Match(r'^\s*#\s*(if|ifdef|ifndef|elif|else|endif)\b', line) + if match: + include_state.ResetSection(match.group(1)) # Make Windows paths like Unix. fullname = os.path.abspath(filename).replace('\\', '/') @@ -4456,7 +4652,7 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, # TODO(unknown): check that 1-arg constructors are explicit. # How to tell it's a constructor? # (handled in CheckForNonStandardConstructs for now) - # TODO(unknown): check that classes have DISALLOW_EVIL_CONSTRUCTORS + # TODO(unknown): check that classes declare or disable copy/assign # (level 1 error) pass @@ -4556,12 +4752,11 @@ def CheckLanguage(filename, clean_lines, linenum, file_extension, 'Do not use variable-length arrays. Use an appropriately named ' "('k' followed by CamelCase) compile-time constant for the size.") - # If DISALLOW_EVIL_CONSTRUCTORS, DISALLOW_COPY_AND_ASSIGN, or - # DISALLOW_IMPLICIT_CONSTRUCTORS is present, then it should be the last thing - # in the class declaration. + # If DISALLOW_COPY_AND_ASSIGN DISALLOW_IMPLICIT_CONSTRUCTORS is present, + # then it should be the last thing in the class declaration. match = Match( (r'\s*' - r'(DISALLOW_(EVIL_CONSTRUCTORS|COPY_AND_ASSIGN|IMPLICIT_CONSTRUCTORS))' + r'(DISALLOW_(COPY_AND_ASSIGN|IMPLICIT_CONSTRUCTORS))' r'\(.*\);$'), line) if match and linenum + 1 < clean_lines.NumLines(): @@ -4599,12 +4794,17 @@ def CheckGlobalStatic(filename, clean_lines, linenum, error): """ line = clean_lines.elided[linenum] + # Match two lines at a time to support multiline declarations + if linenum + 1 < clean_lines.NumLines() and not Search(r'[;({]', line): + line += clean_lines.elided[linenum + 1].strip() + # Check for people declaring static/global STL strings at the top level. # This is dangerous because the C++ language does not guarantee that # globals with constructors are initialized before the first access. match = Match( r'((?:|static +)(?:|const +))string +([a-zA-Z0-9_:]+)\b(.*)', line) + # Remove false positives: # - String pointers (as opposed to values). # string *pointer @@ -4624,7 +4824,7 @@ def CheckGlobalStatic(filename, clean_lines, linenum, error): if (match and not Search(r'\bstring\b(\s+const)?\s*\*\s*(const\s+)?\w', line) and not Search(r'\boperator\W', line) and - not Match(r'\s*(<.*>)?(::[a-zA-Z0-9_]+)?\s*\(([^"]|$)', match.group(3))): + not Match(r'\s*(<.*>)?(::[a-zA-Z0-9_]+)*\s*\(([^"]|$)', match.group(3))): error(filename, linenum, 'runtime/string', 4, 'For a static/global string constant, use a C style string instead: ' '"%schar %s[]".' % @@ -4655,10 +4855,10 @@ def CheckPrintf(filename, clean_lines, linenum, error): 'to snprintf.' % (match.group(1), match.group(2))) # Check if some verboten C functions are being used. - if Search(r'\bsprintf\b', line): + if Search(r'\bsprintf\s*\(', line): error(filename, linenum, 'runtime/printf', 5, 'Never use sprintf. Use snprintf instead.') - match = Search(r'\b(strcpy|strcat)\b', line) + match = Search(r'\b(strcpy|strcat)\s*\(', line) if match: error(filename, linenum, 'runtime/printf', 4, 'Almost always, snprintf is better than %s' % match.group(1)) @@ -4674,13 +4874,16 @@ def IsDerivedFunction(clean_lines, linenum): True if current line contains a function with "override" virt-specifier. """ - # Look for leftmost opening parenthesis on current line - opening_paren = clean_lines.elided[linenum].find('(') - if opening_paren < 0: return False - - # Look for "override" after the matching closing parenthesis - line, _, closing_paren = CloseExpression(clean_lines, linenum, opening_paren) - return closing_paren >= 0 and Search(r'\boverride\b', line[closing_paren:]) + # Scan back a few lines for start of current function + for i in xrange(linenum, max(-1, linenum - 10), -1): + match = Match(r'^([^()]*\w+)\(', clean_lines.elided[i]) + if match: + # Look for "override" after the matching closing parenthesis + line, _, closing_paren = CloseExpression( + clean_lines, i, len(match.group(1))) + return (closing_paren >= 0 and + Search(r'\boverride\b', line[closing_paren:])) + return False def IsInitializerList(clean_lines, linenum): @@ -4806,6 +5009,20 @@ def CheckForNonConstReference(filename, clean_lines, linenum, # Not at toplevel, not within a class, and not within a namespace return + # Avoid initializer lists. We only need to scan back from the + # current line for something that starts with ':'. + # + # We don't need to check the current line, since the '&' would + # appear inside the second set of parentheses on the current line as + # opposed to the first set. + if linenum > 0: + for i in xrange(linenum - 1, max(0, linenum - 10), -1): + previous_line = clean_lines.elided[i] + if not Search(r'[),]\s*$', previous_line): + break + if Match(r'^\s*:\s+\S', previous_line): + return + # Avoid preprocessors if Search(r'\\\s*$', line): return @@ -4881,6 +5098,11 @@ def CheckCasts(filename, clean_lines, linenum, error): # value < double(42) // bracket + space = true positive matched_new_or_template = match.group(1) + # Avoid arrays by looking for brackets that come after the closing + # parenthesis. + if Match(r'\([^()]+\)\s*\[', match.group(3)): + return + # Other things to ignore: # - Function pointers # - Casts to pointer types @@ -4900,28 +5122,35 @@ def CheckCasts(filename, clean_lines, linenum, error): matched_type) if not expecting_function: - CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], - 'static_cast', + CheckCStyleCast(filename, clean_lines, linenum, 'static_cast', r'\((int|float|double|bool|char|u?int(16|32|64))\)', error) # This doesn't catch all cases. Consider (const char * const)"hello". # # (char *) "foo" should always be a const_cast (reinterpret_cast won't # compile). - if CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], - 'const_cast', r'\((char\s?\*+\s?)\)\s*"', error): + if CheckCStyleCast(filename, clean_lines, linenum, 'const_cast', + r'\((char\s?\*+\s?)\)\s*"', error): pass else: # Check pointer casts for other than string constants - CheckCStyleCast(filename, linenum, line, clean_lines.raw_lines[linenum], - 'reinterpret_cast', r'\((\w+\s?\*+\s?)\)', error) + CheckCStyleCast(filename, clean_lines, linenum, 'reinterpret_cast', + r'\((\w+\s?\*+\s?)\)', error) # In addition, we look for people taking the address of a cast. This # is dangerous -- casts can assign to temporaries, so the pointer doesn't # point where you think. + # + # Some non-identifier character is required before the '&' for the + # expression to be recognized as a cast. These are casts: + # expression = &static_cast(temporary()); + # function(&(int*)(temporary())); + # + # This is not a cast: + # reference_type&(int* function_param); match = Search( - r'(?:&\(([^)]+)\)[\w(])|' - r'(?:&(static|dynamic|down|reinterpret)_cast\b)', line) + r'(?:[^\w]&\(([^)]+)\)[\w(])|' + r'(?:[^\w]&(static|dynamic|down|reinterpret)_cast\b)', line) if match and match.group(1) != '*': # Try a better error message when the & is bound to something # dereferenced by the casted pointer, as opposed to the casted @@ -4951,15 +5180,13 @@ def CheckCasts(filename, clean_lines, linenum, error): 'Take the address before doing the cast, rather than after')) -def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, - error): +def CheckCStyleCast(filename, clean_lines, linenum, cast_type, pattern, error): """Checks for a C-style cast by looking for the pattern. Args: filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. linenum: The number of the line to check. - line: The line of code to check. - raw_line: The raw line of code to check, with comments. cast_type: The string for the C++ cast to recommend. This is either reinterpret_cast, static_cast, or const_cast, depending. pattern: The regular expression used to find C-style casts. @@ -4969,19 +5196,26 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, True if an error was emitted. False otherwise. """ + line = clean_lines.elided[linenum] match = Search(pattern, line) if not match: return False - # Exclude lines with keywords that tend to look like casts, and also - # macros which are generally troublesome. - if Match(r'.*\b(?:sizeof|alignof|alignas|[A-Z_]+)\s*$', - line[0:match.start(1) - 1]): + # Exclude lines with keywords that tend to look like casts + context = line[0:match.start(1) - 1] + if Match(r'.*\b(?:sizeof|alignof|alignas|[_A-Z][_A-Z0-9]*)\s*$', context): + return False + + # Try expanding current context to see if we one level of + # parentheses inside a macro. + if linenum > 0: + for i in xrange(linenum - 1, max(0, linenum - 5), -1): + context = clean_lines.elided[i] + context + if Match(r'.*\b[_A-Z][_A-Z0-9]*\s*\((?:\([^()]*\)|[^()])*$', context): return False # operator++(int) and operator--(int) - if (line[0:match.start(1) - 1].endswith(' operator++') or - line[0:match.start(1) - 1].endswith(' operator--')): + if context.endswith(' operator++') or context.endswith(' operator--'): return False # A single unnamed argument for a function tends to look like old @@ -5005,10 +5239,11 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, # (FunctionPointer)(int); # (FunctionPointer)(int) = value; # Function((function_pointer_arg)(int)) + # Function((function_pointer_arg)(int), int param) # ; # <(FunctionPointerTemplateArgument)(int)>; remainder = line[match.end(0):] - if Match(r'^\s*(?:;|const\b|throw\b|final\b|override\b|=|>|\{|\))', + if Match(r'^\s*(?:;|const\b|throw\b|final\b|override\b|[=>{),])', remainder): # Looks like an unnamed parameter. @@ -5031,6 +5266,7 @@ def CheckCStyleCast(filename, linenum, line, raw_line, cast_type, pattern, # Don't warn if the parameter is named with block comments, e.g.: # Function(int /*unused_param*/); + raw_line = clean_lines.raw_lines[linenum] if '/*' in raw_line: return False @@ -5182,16 +5418,16 @@ def FilesBelongToSameModule(filename_cc, filename_h): return files_belong_to_same_module, common_path -def UpdateIncludeState(filename, include_state, io=codecs): - """Fill up the include_state with new includes found from the file. +def UpdateIncludeState(filename, include_dict, io=codecs): + """Fill up the include_dict with new includes found from the file. Args: filename: the name of the header to read. - include_state: an _IncludeState instance in which the headers are inserted. + include_dict: a dictionary in which the headers are inserted. io: The io factory to use to read the file. Provided for testability. Returns: - True if a header was succesfully added. False otherwise. + True if a header was successfully added. False otherwise. """ headerfile = None try: @@ -5205,9 +5441,7 @@ def UpdateIncludeState(filename, include_state, io=codecs): match = _RE_PATTERN_INCLUDE.search(clean_line) if match: include = match.group(2) - # The value formatting is cute, but not really used right now. - # What matters here is that the key is in include_state. - include_state.setdefault(include, '%s:%d' % (filename, linenum)) + include_dict.setdefault(include, linenum) return True @@ -5260,10 +5494,11 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, # The policy is that if you #include something in foo.h you don't need to # include it again in foo.cc. Here, we will look at possible includes. - # Let's copy the include_state so it is only messed up within this function. - include_state = include_state.copy() + # Let's flatten the include_state include_list and copy it into a dictionary. + include_dict = dict([item for sublist in include_state.include_list + for item in sublist]) - # Did we find the header for this file (if any) and succesfully load it? + # Did we find the header for this file (if any) and successfully load it? header_found = False # Use the absolute path so that matching works properly. @@ -5278,13 +5513,13 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, # instead of 'foo_flymake.h' abs_filename = re.sub(r'_flymake\.cc$', '.cc', abs_filename) - # include_state is modified during iteration, so we iterate over a copy of + # include_dict is modified during iteration, so we iterate over a copy of # the keys. - header_keys = include_state.keys() + header_keys = include_dict.keys() for header in header_keys: (same_module, common_path) = FilesBelongToSameModule(abs_filename, header) fullpath = common_path + header - if same_module and UpdateIncludeState(fullpath, include_state, io): + if same_module and UpdateIncludeState(fullpath, include_dict, io): header_found = True # If we can't find the header file for a .cc, assume it's because we don't @@ -5298,7 +5533,7 @@ def CheckForIncludeWhatYouUse(filename, clean_lines, include_state, error, # All the lines have been processed, report the errors found. for required_header_unstripped in required: template = required[required_header_unstripped][1] - if required_header_unstripped.strip('<>"') not in include_state: + if required_header_unstripped.strip('<>"') not in include_dict: error(filename, required[required_header_unstripped][0], 'build/include_what_you_use', 4, 'Add #include ' + required_header_unstripped + ' for ' + template) @@ -5326,6 +5561,8 @@ def CheckMakePairUsesDeduction(filename, clean_lines, linenum, error): 4, # 4 = high confidence 'For C++11-compatibility, omit template arguments from make_pair' ' OR use pair directly OR if appropriate, construct a pair directly') + + def CheckDefaultLambdaCaptures(filename, clean_lines, linenum, error): """Check that default lambda captures are not used. @@ -5351,6 +5588,139 @@ def CheckDefaultLambdaCaptures(filename, clean_lines, linenum, error): 'Default lambda captures are an unapproved C++ feature.') +def CheckRedundantVirtual(filename, clean_lines, linenum, error): + """Check if line contains a redundant "virtual" function-specifier. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + # Look for "virtual" on current line. + line = clean_lines.elided[linenum] + virtual = Match(r'^(.*\bvirtual\b)', line) + if not virtual: return + + # Look for the next opening parenthesis. This is the start of the + # parameter list (possibly on the next line shortly after virtual). + # TODO(unknown): doesn't work if there are virtual functions with + # decltype() or other things that use parentheses, but csearch suggests + # that this is rare. + end_col = -1 + end_line = -1 + start_col = len(virtual.group(1)) + for start_line in xrange(linenum, min(linenum + 3, clean_lines.NumLines())): + line = clean_lines.elided[start_line][start_col:] + parameter_list = Match(r'^([^(]*)\(', line) + if parameter_list: + # Match parentheses to find the end of the parameter list + (_, end_line, end_col) = CloseExpression( + clean_lines, start_line, start_col + len(parameter_list.group(1))) + break + start_col = 0 + + if end_col < 0: + return # Couldn't find end of parameter list, give up + + # Look for "override" or "final" after the parameter list + # (possibly on the next few lines). + for i in xrange(end_line, min(end_line + 3, clean_lines.NumLines())): + line = clean_lines.elided[i][end_col:] + match = Search(r'\b(override|final)\b', line) + if match: + error(filename, linenum, 'readability/inheritance', 4, + ('"virtual" is redundant since function is ' + 'already declared as "%s"' % match.group(1))) + + # Set end_col to check whole lines after we are done with the + # first line. + end_col = 0 + if Search(r'[^\w]\s*$', line): + break + + +def CheckRedundantOverrideOrFinal(filename, clean_lines, linenum, error): + """Check if line contains a redundant "override" or "final" virt-specifier. + + Args: + filename: The name of the current file. + clean_lines: A CleansedLines instance containing the file. + linenum: The number of the line to check. + error: The function to call with any errors found. + """ + # Check that at most one of "override" or "final" is present, not both + line = clean_lines.elided[linenum] + if Search(r'\boverride\b', line) and Search(r'\bfinal\b', line): + error(filename, linenum, 'readability/inheritance', 4, + ('"override" is redundant since function is ' + 'already declared as "final"')) + + + + +# Returns true if we are at a new block, and it is directly +# inside of a namespace. +def IsBlockInNameSpace(nesting_state, is_forward_declaration): + """Checks that the new block is directly in a namespace. + + Args: + nesting_state: The _NestingState object that contains info about our state. + is_forward_declaration: If the class is a forward declared class. + Returns: + Whether or not the new block is directly in a namespace. + """ + if is_forward_declaration: + if len(nesting_state.stack) >= 1 and ( + isinstance(nesting_state.stack[-1], _NamespaceInfo)): + return True + else: + return False + + return (len(nesting_state.stack) > 1 and + nesting_state.stack[-1].check_namespace_indentation and + isinstance(nesting_state.stack[-2], _NamespaceInfo)) + + +def ShouldCheckNamespaceIndentation(nesting_state, is_namespace_indent_item, + raw_lines_no_comments, linenum): + """This method determines if we should apply our namespace indentation check. + + Args: + nesting_state: The current nesting state. + is_namespace_indent_item: If we just put a new class on the stack, True. + If the top of the stack is not a class, or we did not recently + add the class, False. + raw_lines_no_comments: The lines without the comments. + linenum: The current line number we are processing. + + Returns: + True if we should apply our namespace indentation check. Currently, it + only works for classes and namespaces inside of a namespace. + """ + + is_forward_declaration = IsForwardClassDeclaration(raw_lines_no_comments, + linenum) + + if not (is_namespace_indent_item or is_forward_declaration): + return False + + # If we are in a macro, we do not want to check the namespace indentation. + if IsMacroDefinition(raw_lines_no_comments, linenum): + return False + + return IsBlockInNameSpace(nesting_state, is_forward_declaration) + + +# Call this method if the line is directly inside of a namespace. +# If the line above is blank (excluding comments) or the start of +# an inner namespace, it cannot be indented. +def CheckItemIndentationInNamespace(filename, raw_lines_no_comments, linenum, + error): + line = raw_lines_no_comments[linenum] + if Match(r'^\s+', line): + error(filename, linenum, 'runtime/indentation_namespace', 4, + 'Do not indent within a namespace') def ProcessLine(filename, file_extension, clean_lines, line, @@ -5377,6 +5747,8 @@ def ProcessLine(filename, file_extension, clean_lines, line, raw_lines = clean_lines.raw_lines ParseNolintSuppressions(filename, raw_lines[line], line, error) nesting_state.Update(filename, clean_lines, line, error) + CheckForNamespaceIndentation(filename, nesting_state, clean_lines, line, + error) if nesting_state.InAsmBlock(): return CheckForFunctionLengths(filename, clean_lines, line, function_state, error) CheckForMultilineCommentsAndStrings(filename, clean_lines, line, error) @@ -5391,6 +5763,8 @@ def ProcessLine(filename, file_extension, clean_lines, line, CheckInvalidIncrement(filename, clean_lines, line, error) CheckMakePairUsesDeduction(filename, clean_lines, line, error) CheckDefaultLambdaCaptures(filename, clean_lines, line, error) + CheckRedundantVirtual(filename, clean_lines, line, error) + CheckRedundantOverrideOrFinal(filename, clean_lines, line, error) for check_fn in extra_check_functions: check_fn(filename, clean_lines, line, error) diff --git a/cpplint/cpplint_unittest.py b/cpplint/cpplint_unittest.py index 8a66def..c380696 100755 --- a/cpplint/cpplint_unittest.py +++ b/cpplint/cpplint_unittest.py @@ -37,14 +37,16 @@ import codecs import os import random import re +import sys import unittest + import cpplint # This class works as an error collector and replaces cpplint.Error # function for the unit tests. We also verify each category we see # is in cpplint._ERROR_CATEGORIES, to help keep that list up to date. -class ErrorCollector: +class ErrorCollector(object): # These are a global list, covering all categories seen ever. _ERROR_CATEGORIES = cpplint._ERROR_CATEGORIES _SEEN_ERROR_CATEGORIES = {} @@ -74,7 +76,7 @@ class ErrorCollector: return self._errors def VerifyAllCategoriesAreSeen(self): - """Fails if there's a category in _ERROR_CATEGORIES - _SEEN_ERROR_CATEGORIES. + """Fails if there's a category in _ERROR_CATEGORIES~_SEEN_ERROR_CATEGORIES. This should only be called after all tests are run, so _SEEN_ERROR_CATEGORIES has had a chance to fully populate. Since @@ -84,7 +86,6 @@ class ErrorCollector: """ for category in self._ERROR_CATEGORIES: if category not in self._SEEN_ERROR_CATEGORIES: - import sys sys.exit('FATAL ERROR: There are no tests for category "%s"' % category) def RemoveIfPresent(self, substr): @@ -96,7 +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: +class MockIo(object): def __init__(self, mock_file): self.mock_file = mock_file @@ -109,6 +110,13 @@ class MockIo: class CpplintTestBase(unittest.TestCase): """Provides some useful helper functions for cpplint tests.""" + def setUp(self): + # Allow subclasses to cheat os.path.abspath called in FileInfo class. + self.os_path_abspath_orig = os.path.abspath + + def tearDown(self): + os.path.abspath = self.os_path_abspath_orig + # Perform lint on single line of input and return the error message. def PerformSingleLineLint(self, code): error_collector = ErrorCollector(self.assert_) @@ -376,6 +384,15 @@ class CpplintTest(CpplintTestBase): 'long a = 65; // NOLINT(readability/casting)', 'Use int16/int64/etc, rather than the C type long' ' [runtime/int] [4]') + # NOLINTNEXTLINE silences warning for the next line instead of current line + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData('test.cc', 'cc', + ['// Copyright 2014 Your Company.', + '// NOLINTNEXTLINE(whitespace/line_length)', + '// ./command' + (' -verbose' * 80), + ''], + error_collector) + self.assertEquals('', error_collector.Results()) # Test Variable Declarations. def testVariableDeclarations(self): @@ -499,8 +516,9 @@ class CpplintTest(CpplintTestBase): 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 {', '') + self.TestLint('virtual void f(int /*unused*/) {', '') + self.TestLint('void f(int /*unused*/) override {', '') + self.TestLint('void f(int /*unused*/) final {', '') # Test deprecated casts such as int(d) def testDeprecatedCast(self): @@ -523,81 +541,48 @@ class CpplintTest(CpplintTestBase): ' [readability/casting] [4]') # Checks for false positives... - self.TestLint( - 'int a = int(); // Constructor, o.k.', - '') - self.TestLint( - 'X::X() : a(int()) {} // default Constructor, o.k.', - '') - self.TestLint( - 'operator bool(); // Conversion operator, o.k.', - '') - self.TestLint( - 'new int64(123); // "new" operator on basic type, o.k.', - '') - self.TestLint( - 'new int64(123); // "new" operator on basic type, weird spacing', - '') - 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('int a = int();', '') # constructor + self.TestLint('X::X() : a(int()) {}', '') # default constructor + self.TestLint('operator bool();', '') # Conversion operator + self.TestLint('new int64(123);', '') # "new" operator on basic type + self.TestLint('new int64(123);', '') # "new" operator on basic type + self.TestLint('using a = bool(int arg);', '') # C++11 alias-declaration + self.TestLint('x = bit_cast(y);', '') # array of array + self.TestLint('void F(const char(&src)[N]);', '') # array of references + + # Placement new self.TestLint( 'new(field_ptr) int(field->default_value_enum()->number());', '') + # C++11 function wrappers + self.TestLint('std::function', '') + self.TestLint('std::function', '') + self.TestLint('std::function< int(bool) >', '') + self.TestLint('mfunction', '') + error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'test.cc', 'cc', - ['// Copyright 2008 Your Company. All Rights Reserved.', + ['// Copyright 2014 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( - 'typedef bool(FunctionPointer)();', - '') - self.TestLint( - 'typedef bool(FunctionPointer)(int param);', - '') - self.TestLint( - 'typedef bool(MyClass::*MemberFunctionPointer)();', - '') - self.TestLint( - 'typedef bool(MyClass::* MemberFunctionPointer)();', - '') - self.TestLint( - 'typedef bool(MyClass::*MemberFunctionPointer)() const;', - '') - self.TestLint( - 'void Function(bool(FunctionPointerArg)());', - '') - self.TestLint( - 'void Function(bool(FunctionPointerArg)()) {}', - '') - self.TestLint( - 'typedef set SortedIdSet', - '') + # Return types for function pointers + self.TestLint('typedef bool(FunctionPointer)();', '') + self.TestLint('typedef bool(FunctionPointer)(int param);', '') + self.TestLint('typedef bool(MyClass::*MemberFunctionPointer)();', '') + self.TestLint('typedef bool(MyClass::* MemberFunctionPointer)();', '') + self.TestLint('typedef bool(MyClass::*MemberFunctionPointer)() const;', '') + self.TestLint('void Function(bool(FunctionPointerArg)());', '') + self.TestLint('void Function(bool(FunctionPointerArg)()) {}', '') + self.TestLint('typedef set SortedIdSet', '') 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. @@ -991,6 +976,8 @@ class CpplintTest(CpplintTestBase): 'Could not find end of multi-line comment' ' [readability/multiline_comment] [5]') self.TestMultiLineLint(r""" // /* comment, but not multi-line""", '') + self.TestMultiLineLint(r"""/********** + */""", '') def testMultilineStrings(self): multiline_string_error_message = ( @@ -1012,192 +999,283 @@ class CpplintTest(CpplintTestBase): # Test non-explicit single-argument constructors def testExplicitSingleArgumentConstructors(self): - # missing explicit is bad - self.TestMultiLineLint( - """ - class Foo { - Foo(int f); - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # missing explicit is bad, even with whitespace - self.TestMultiLineLint( - """ - class Foo { - Foo (int f); - };""", - ['Extra space before ( in function call [whitespace/parens] [4]', - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]']) - # missing explicit, with distracting comment, is still bad - self.TestMultiLineLint( - """ - class Foo { - Foo(int f); // simpler than Foo(blargh, blarg) - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # missing explicit, with qualified classname - self.TestMultiLineLint( - """ - class Qualifier::AnotherOne::Foo { - Foo(int f); - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # missing explicit for inline constructors is bad as well - self.TestMultiLineLint( - """ - class Foo { - inline Foo(int f); - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # structs are caught as well. - self.TestMultiLineLint( - """ - struct Foo { - Foo(int f); - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # Templatized classes are caught as well. - self.TestMultiLineLint( - """ - template class Foo { - Foo(int f); - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # inline case for templatized classes. - self.TestMultiLineLint( - """ - template class Foo { - inline Foo(int f); - };""", - 'Single-argument constructors should be marked explicit.' - ' [runtime/explicit] [5]') - # proper style is okay - self.TestMultiLineLint( - """ - class Foo { - explicit Foo(int f); - };""", - '') - # two argument constructor is okay - self.TestMultiLineLint( - """ - class Foo { - Foo(int f, int b); - };""", - '') - # two argument constructor, across two lines, is okay - self.TestMultiLineLint( - """ - class Foo { - Foo(int f, - int b); - };""", - '') - # non-constructor (but similar name), is okay - self.TestMultiLineLint( - """ - class Foo { - aFoo(int f); - };""", - '') - # constructor with void argument is okay - self.TestMultiLineLint( - """ - class Foo { - Foo(void); - };""", - '') - # single argument method is okay - self.TestMultiLineLint( - """ - class Foo { - Bar(int b); - };""", - '') - # comments should be ignored - self.TestMultiLineLint( - """ - class Foo { - // Foo(int f); - };""", - '') - # single argument function following class definition is okay - # (okay, it's not actually valid, but we don't want a false positive) - self.TestMultiLineLint( - """ - class Foo { - Foo(int f, int b); - }; - Foo(int f);""", - '') - # single argument function is okay - self.TestMultiLineLint( - """static Foo(int f);""", - '') - # single argument copy constructor is okay. - self.TestMultiLineLint( - """ - class Foo { - Foo(const Foo&); - };""", - '') - self.TestMultiLineLint( - """ - class Foo { - Foo(Foo const&); - };""", - '') - self.TestMultiLineLint( - """ - class Foo { - Foo(Foo&); - };""", - '') - # templatized copy constructor is okay. - self.TestMultiLineLint( - """ - template class Foo { - 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', - ['void Func() {', - ' __asm__ (', - ' "hlt"', - ' );', - ' asm {', - ' movdqa [edx + 32], xmm2', - ' }', - '}'], - error_collector) - self.assertEquals( - 0, - error_collector.ResultList().count( - 'Extra space before ( in function call [whitespace/parens] [4]')) - self.assertEquals( - 0, - error_collector.ResultList().count( - 'Closing ) should be moved to the previous line ' - '[whitespace/parens] [2]')) - self.assertEquals( - 0, - error_collector.ResultList().count( - 'Extra space before [ [whitespace/braces] [5]')) + old_verbose_level = cpplint._cpplint_state.verbose_level + cpplint._cpplint_state.verbose_level = 0 + + try: + # missing explicit is bad + self.TestMultiLineLint( + """ + class Foo { + Foo(int f); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # missing explicit is bad, even with whitespace + self.TestMultiLineLint( + """ + class Foo { + Foo (int f); + };""", + ['Extra space before ( in function call [whitespace/parens] [4]', + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]']) + # missing explicit, with distracting comment, is still bad + self.TestMultiLineLint( + """ + class Foo { + Foo(int f); // simpler than Foo(blargh, blarg) + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # missing explicit, with qualified classname + self.TestMultiLineLint( + """ + class Qualifier::AnotherOne::Foo { + Foo(int f); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # missing explicit for inline constructors is bad as well + self.TestMultiLineLint( + """ + class Foo { + inline Foo(int f); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # structs are caught as well. + self.TestMultiLineLint( + """ + struct Foo { + Foo(int f); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # Templatized classes are caught as well. + self.TestMultiLineLint( + """ + template class Foo { + Foo(int f); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # inline case for templatized classes. + self.TestMultiLineLint( + """ + template class Foo { + inline Foo(int f); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # constructors with a default argument should still be marked explicit + self.TestMultiLineLint( + """ + class Foo { + Foo(int f = 0); + };""", + 'Constructors callable with one argument should be marked explicit.' + ' [runtime/explicit] [5]') + # multi-argument constructors with all but one default argument should be + # marked explicit + self.TestMultiLineLint( + """ + class Foo { + Foo(int f, int g = 0); + };""", + 'Constructors callable with one argument should be marked explicit.' + ' [runtime/explicit] [5]') + # multi-argument constructors with all default arguments should be marked + # explicit + self.TestMultiLineLint( + """ + class Foo { + Foo(int f = 0, int g = 0); + };""", + 'Constructors callable with one argument should be marked explicit.' + ' [runtime/explicit] [5]') + # explicit no-argument constructors are bad + self.TestMultiLineLint( + """ + class Foo { + explicit Foo(); + };""", + 'Zero-parameter constructors should not be marked explicit.' + ' [runtime/explicit] [5]') + # void constructors are considered no-argument + self.TestMultiLineLint( + """ + class Foo { + explicit Foo(void); + };""", + 'Zero-parameter constructors should not be marked explicit.' + ' [runtime/explicit] [5]') + # Warn explicit multi-argument constructors at lowest severity + self.TestMultiLineLint( + """ + class Foo { + explicit Foo(int f, int g); + };""", + 'Constructors that require multiple arguments ' + 'should not be marked explicit. [runtime/explicit] [0]') + # but explicit multi-argument constructors with only one non-default + # argument are OK + self.TestMultiLineLint( + """ + class Foo { + explicit Foo(int f, int g = 0); + };""", + '') + # single-argument constructors that take a function that takes multiple + # arguments should be explicit + self.TestMultiLineLint( + """ + class Foo { + Foo(void (*f)(int f, int g)); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # single-argument constructors that take a single template argument with + # multiple parameters should be explicit + self.TestMultiLineLint( + """ + template + class Foo { + Foo(Bar b); + };""", + 'Single-parameter constructors should be marked explicit.' + ' [runtime/explicit] [5]') + # but copy constructors that take multiple template parameters are OK + self.TestMultiLineLint( + """ + template + class Foo { + Foo(Foo& f); + };""", + '') + # proper style is okay + self.TestMultiLineLint( + """ + class Foo { + explicit Foo(int f); + };""", + '') + # two argument constructor is okay + self.TestMultiLineLint( + """ + class Foo { + Foo(int f, int b); + };""", + '') + # two argument constructor, across two lines, is okay + self.TestMultiLineLint( + """ + class Foo { + Foo(int f, + int b); + };""", + '') + # non-constructor (but similar name), is okay + self.TestMultiLineLint( + """ + class Foo { + aFoo(int f); + };""", + '') + # constructor with void argument is okay + self.TestMultiLineLint( + """ + class Foo { + Foo(void); + };""", + '') + # single argument method is okay + self.TestMultiLineLint( + """ + class Foo { + Bar(int b); + };""", + '') + # comments should be ignored + self.TestMultiLineLint( + """ + class Foo { + // Foo(int f); + };""", + '') + # single argument function following class definition is okay + # (okay, it's not actually valid, but we don't want a false positive) + self.TestMultiLineLint( + """ + class Foo { + Foo(int f, int b); + }; + Foo(int f);""", + '') + # single argument function is okay + self.TestMultiLineLint( + """static Foo(int f);""", + '') + # single argument copy constructor is okay. + self.TestMultiLineLint( + """ + class Foo { + Foo(const Foo&); + };""", + '') + self.TestMultiLineLint( + """ + class Foo { + Foo(Foo const&); + };""", + '') + self.TestMultiLineLint( + """ + class Foo { + Foo(Foo&); + };""", + '') + # templatized copy constructor is okay. + self.TestMultiLineLint( + """ + template class Foo { + 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', + ['void Func() {', + ' __asm__ (', + ' "hlt"', + ' );', + ' asm {', + ' movdqa [edx + 32], xmm2', + ' }', + '}'], + error_collector) + self.assertEquals( + 0, + error_collector.ResultList().count( + 'Extra space before ( in function call [whitespace/parens] [4]')) + self.assertEquals( + 0, + error_collector.ResultList().count( + 'Closing ) should be moved to the previous line ' + '[whitespace/parens] [2]')) + self.assertEquals( + 0, + error_collector.ResultList().count( + 'Extra space before [ [whitespace/braces] [5]')) + finally: + cpplint._cpplint_state.verbose_level = old_verbose_level def testSlashStarCommentOnSingleLine(self): self.TestMultiLineLint( @@ -1269,6 +1347,44 @@ class CpplintTest(CpplintTestBase): ' memset(buf, 0xcd, 0)', '') + def testRedundantVirtual(self): + self.TestLint('virtual void F()', '') + self.TestLint('virtual void F();', '') + self.TestLint('virtual void F() {}', '') + + message_template = ('"%s" is redundant since function is already ' + 'declared as "%s" [readability/inheritance] [4]') + for virt_specifier in ['override', 'final']: + error_message = message_template % ('virtual', virt_specifier) + self.TestLint('virtual int F() %s' % virt_specifier, error_message) + self.TestLint('virtual int F() %s;' % virt_specifier, error_message) + self.TestLint('virtual int F() %s {' % virt_specifier, error_message) + + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData( + 'foo.cc', 'cc', + ['// Copyright 2014 Your Company.', + 'virtual void F(int a,', + ' int b) ' + virt_specifier + ';', + 'virtual void F(int a,', + ' int b) LOCKS_EXCLUDED(lock) ' + virt_specifier + ';', + 'virtual void F(int a,', + ' int b)', + ' LOCKS_EXCLUDED(lock) ' + virt_specifier + ';', + ''], + error_collector) + self.assertEquals( + [error_message, error_message, error_message], + error_collector.Results()) + + error_message = message_template % ('override', 'final') + self.TestLint('int F() override final', error_message) + self.TestLint('int F() override final;', error_message) + self.TestLint('int F() override final {}', error_message) + self.TestLint('int F() final override', error_message) + self.TestLint('int F() final override;', error_message) + self.TestLint('int F() final override {}', error_message) + def testCheckDeprecated(self): self.TestLanguageRulesCheck('foo.cc', '#include ', 'Streams are highly discouraged.' @@ -1423,11 +1539,10 @@ class CpplintTest(CpplintTestBase): self.TestLint('delete a[some_var];', '') self.TestLint('return a[some_var];', '') - # DISALLOW_EVIL_CONSTRUCTORS should be at end of class if present. - # Same with DISALLOW_COPY_AND_ASSIGN and DISALLOW_IMPLICIT_CONSTRUCTORS. - def testDisallowEvilConstructors(self): + # DISALLOW_COPY_AND_ASSIGN and DISALLOW_IMPLICIT_CONSTRUCTORS should be at + # end of class if present. + def testDisallowMacrosAtEnd(self): for macro_name in ( - 'DISALLOW_EVIL_CONSTRUCTORS', 'DISALLOW_COPY_AND_ASSIGN', 'DISALLOW_IMPLICIT_CONSTRUCTORS'): self.TestLanguageRulesCheck( @@ -1458,7 +1573,6 @@ class CpplintTest(CpplintTestBase): # DISALLOW* macros should be in the private: section. def testMisplacedDisallowMacros(self): for macro_name in ( - 'DISALLOW_EVIL_CONSTRUCTORS', 'DISALLOW_COPY_AND_ASSIGN', 'DISALLOW_IMPLICIT_CONSTRUCTORS'): self.TestMultiLineLint( @@ -1507,17 +1621,17 @@ class CpplintTest(CpplintTestBase): """ class Outer3 { struct Inner3 { - DISALLOW_EVIL_CONSTRUCTORS(Inner3); + DISALLOW_COPY_AND_ASSIGN(Inner3); }; DISALLOW_IMPLICIT_CONSTRUCTORS(Outer3); };""", - ('DISALLOW_EVIL_CONSTRUCTORS must be in the private: section' + ('DISALLOW_COPY_AND_ASSIGN must be in the private: section' ' [readability/constructors] [3]')) self.TestMultiLineLint( """ struct Outer4 { class Inner4 { - DISALLOW_EVIL_CONSTRUCTORS(Inner4); + DISALLOW_COPY_AND_ASSIGN(Inner4); }; DISALLOW_IMPLICIT_CONSTRUCTORS(Outer4); };""", @@ -1551,100 +1665,100 @@ class CpplintTest(CpplintTestBase): # CHECK/EXPECT_TRUE/EXPECT_FALSE replacements def testCheckCheck(self): - self.TestLint('CHECK(x == 42)', + self.TestLint('CHECK(x == 42);', 'Consider using CHECK_EQ instead of CHECK(a == b)' ' [readability/check] [2]') - self.TestLint('CHECK(x != 42)', + self.TestLint('CHECK(x != 42);', 'Consider using CHECK_NE instead of CHECK(a != b)' ' [readability/check] [2]') - self.TestLint('CHECK(x >= 42)', + self.TestLint('CHECK(x >= 42);', 'Consider using CHECK_GE instead of CHECK(a >= b)' ' [readability/check] [2]') - self.TestLint('CHECK(x > 42)', + self.TestLint('CHECK(x > 42);', 'Consider using CHECK_GT instead of CHECK(a > b)' ' [readability/check] [2]') - self.TestLint('CHECK(x <= 42)', + self.TestLint('CHECK(x <= 42);', 'Consider using CHECK_LE instead of CHECK(a <= b)' ' [readability/check] [2]') - self.TestLint('CHECK(x < 42)', + self.TestLint('CHECK(x < 42);', 'Consider using CHECK_LT instead of CHECK(a < b)' ' [readability/check] [2]') - self.TestLint('DCHECK(x == 42)', + self.TestLint('DCHECK(x == 42);', 'Consider using DCHECK_EQ instead of DCHECK(a == b)' ' [readability/check] [2]') - self.TestLint('DCHECK(x != 42)', + self.TestLint('DCHECK(x != 42);', 'Consider using DCHECK_NE instead of DCHECK(a != b)' ' [readability/check] [2]') - self.TestLint('DCHECK(x >= 42)', + self.TestLint('DCHECK(x >= 42);', 'Consider using DCHECK_GE instead of DCHECK(a >= b)' ' [readability/check] [2]') - self.TestLint('DCHECK(x > 42)', + self.TestLint('DCHECK(x > 42);', 'Consider using DCHECK_GT instead of DCHECK(a > b)' ' [readability/check] [2]') - self.TestLint('DCHECK(x <= 42)', + self.TestLint('DCHECK(x <= 42);', 'Consider using DCHECK_LE instead of DCHECK(a <= b)' ' [readability/check] [2]') - self.TestLint('DCHECK(x < 42)', + self.TestLint('DCHECK(x < 42);', 'Consider using DCHECK_LT instead of DCHECK(a < b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE("42" == x)', + 'EXPECT_TRUE("42" == x);', 'Consider using EXPECT_EQ instead of EXPECT_TRUE(a == b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE("42" != x)', + 'EXPECT_TRUE("42" != x);', 'Consider using EXPECT_NE instead of EXPECT_TRUE(a != b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE(+42 >= x)', + 'EXPECT_TRUE(+42 >= x);', 'Consider using EXPECT_GE instead of EXPECT_TRUE(a >= b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE_M(-42 > x)', + 'EXPECT_TRUE_M(-42 > x);', 'Consider using EXPECT_GT_M instead of EXPECT_TRUE_M(a > b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE_M(42U <= x)', + 'EXPECT_TRUE_M(42U <= x);', 'Consider using EXPECT_LE_M instead of EXPECT_TRUE_M(a <= b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE_M(42L < x)', + 'EXPECT_TRUE_M(42L < x);', 'Consider using EXPECT_LT_M instead of EXPECT_TRUE_M(a < b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_FALSE(x == 42)', + 'EXPECT_FALSE(x == 42);', 'Consider using EXPECT_NE instead of EXPECT_FALSE(a == b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_FALSE(x != 42)', + 'EXPECT_FALSE(x != 42);', 'Consider using EXPECT_EQ instead of EXPECT_FALSE(a != b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_FALSE(x >= 42)', + 'EXPECT_FALSE(x >= 42);', 'Consider using EXPECT_LT instead of EXPECT_FALSE(a >= b)' ' [readability/check] [2]') self.TestLint( - 'ASSERT_FALSE(x > 42)', + 'ASSERT_FALSE(x > 42);', 'Consider using ASSERT_LE instead of ASSERT_FALSE(a > b)' ' [readability/check] [2]') self.TestLint( - 'ASSERT_FALSE(x <= 42)', + 'ASSERT_FALSE(x <= 42);', 'Consider using ASSERT_GT instead of ASSERT_FALSE(a <= b)' ' [readability/check] [2]') self.TestLint( - 'ASSERT_FALSE_M(x < 42)', + 'ASSERT_FALSE_M(x < 42);', 'Consider using ASSERT_GE_M instead of ASSERT_FALSE_M(a < b)' ' [readability/check] [2]') - self.TestLint('CHECK(x<42)', + self.TestLint('CHECK(x<42);', ['Missing spaces around <' ' [whitespace/operators] [3]', 'Consider using CHECK_LT instead of CHECK(a < b)' ' [readability/check] [2]']) - self.TestLint('CHECK(x>42)', + self.TestLint('CHECK(x>42);', ['Missing spaces around >' ' [whitespace/operators] [3]', 'Consider using CHECK_GT instead of CHECK(a > b)' @@ -1653,41 +1767,42 @@ 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->y == 42);', 'Consider using CHECK_EQ instead of CHECK(a == b)' ' [readability/check] [2]') self.TestLint( - ' EXPECT_TRUE(42 < x) // Random comment.', + ' EXPECT_TRUE(42 < x); // Random comment.', 'Consider using EXPECT_LT instead of EXPECT_TRUE(a < b)' ' [readability/check] [2]') self.TestLint( - 'EXPECT_TRUE( 42 < x )', + 'EXPECT_TRUE( 42 < x );', ['Extra space after ( in function call' ' [whitespace/parens] [4]', + 'Extra space before ) [whitespace/parens] [2]', 'Consider using EXPECT_LT instead of EXPECT_TRUE(a < b)' ' [readability/check] [2]']) - self.TestLint('CHECK(4\'2 == x)', + 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(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.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); @@ -1703,6 +1818,8 @@ class CpplintTest(CpplintTestBase): _STLP_DEFINE_BINARY_OP_CHECK(%, _OP_MOD);""", '') + self.TestLint('CHECK(x < 42) << "Custom error message";', '') + # Alternative token to punctuation operator replacements def testCheckAltTokens(self): self.TestLint('true or true', @@ -1822,12 +1939,20 @@ class CpplintTest(CpplintTestBase): 'vector &nonconst_x) {', operand_error_message % 'vector &nonconst_x') + # Derived member functions are spared from override check + self.TestLint('void Func(X& x);', operand_error_message % 'X& x') + self.TestLint('void Func(X& x) {}', operand_error_message % 'X& x') + self.TestLint('void Func(X& x) override;', '') + self.TestLint('void Func(X& x) override {', '') + self.TestLint('void Func(X& x) const override;', '') + self.TestLint('void Func(X& x) const override {', '') + # Other potential false positives. These need full parser # state to reproduce as opposed to just TestLint. error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'foo.cc', 'cc', - ['// Copyright 2008 Your Company. All Rights Reserved.', + ['// Copyright 2014 Your Company. All Rights Reserved.', 'void swap(int &x,', ' int &y) {', '}', @@ -1839,18 +1964,24 @@ class CpplintTest(CpplintTestBase): ' ostream& out', ' const dense_hash_set& seq) {', '}', + 'void Derived::Function(', + ' string &x) override {', + '}', '#define UNSUPPORTED_MASK(_mask) \\', ' if (flags & _mask) { \\', ' LOG(FATAL) << "Unsupported flag: " << #_mask; \\', ' }', 'Constructor::Constructor()', - ' : initializer1_(a & b),', - ' initializer2_(a & b) {', + ' : initializer1_(a1 & b1),', + ' initializer2_(a2 & b2) {', '}', 'Constructor::Constructor()', - ' : initializer1_{a & b},', - ' initializer2_(a & b) {', + ' : initializer1_{a3 & b3},', + ' initializer2_(a4 & b4) {', '}', + 'Constructor::Constructor()', + ' : initializer1_{a5 & b5},', + ' initializer2_(a6 & b6) {}', ''], error_collector) self.assertEquals('', error_collector.Results()) @@ -1859,7 +1990,7 @@ class CpplintTest(CpplintTestBase): error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'foo.cc', 'cc', - ['// Copyright 2008 Your Company. All Rights Reserved.', + ['// Copyright 2014 Your Company. All Rights Reserved.', 'void Func(const Outer::', ' Inner& const_x,', ' const Outer', @@ -1885,7 +2016,7 @@ class CpplintTest(CpplintTestBase): error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'foo.cc', 'cc', - ['// Copyright 2008 Your Company. All Rights Reserved.', + ['// Copyright 2014 Your Company. All Rights Reserved.', 'inline RCULocked::ReadPtr::ReadPtr(const RCULocked* rcu) {', ' DCHECK(!(data & kFlagMask)) << "Error";', '}', @@ -2246,10 +2377,11 @@ class CpplintTest(CpplintTestBase): self.TestLint('A(a b) : c(d&& e)', space_error) self.TestLint('A(a b) : c(), d(e&& f)', space_error) + def testAllowedRvalueReference(self): # 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.', + ['// Copyright 2014 Your Company.', 'GOOGLE_ALLOW_RVALUE_REFERENCES_PUSH', 'void F(A&& b);', 'GOOGLE_ALLOW_RVALUE_REFERENCES_POP', @@ -2257,6 +2389,22 @@ class CpplintTest(CpplintTestBase): error_collector) self.assertEquals(error_collector.Results(), '') + # RValue references for constructors and operator= + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData( + 'foo.cc', 'cc', + ['// Copyright 2014 Your Company.', + 'class X {', + ' X(X&& param) = delete; // NOLINT(runtime/explicit)', + ' X(X &¶m) = default; // NOLINT(runtime/explicit)', + '', + ' X& operator=(X&& param) = delete;', + ' X& operator=(X&& param)=default;', + '};', + ''], + error_collector) + self.assertEquals(error_collector.Results(), '') + def testSpacingBeforeLastSemicolon(self): self.TestLint('call_function() ;', 'Extra space before last semicolon. If this should be an ' @@ -2334,26 +2482,19 @@ class CpplintTest(CpplintTestBase): # Static or global STL strings. def testStaticOrGlobalSTLStrings(self): + error_msg = ('For a static/global string constant, use a C style ' + 'string instead: "%s[]". [runtime/string] [4]') + self.TestLint('string foo;', - 'For a static/global string constant, use a C style ' - 'string instead: "char foo[]".' - ' [runtime/string] [4]') + error_msg % 'char foo') self.TestLint('string kFoo = "hello"; // English', - 'For a static/global string constant, use a C style ' - 'string instead: "char kFoo[]".' - ' [runtime/string] [4]') + error_msg % 'char kFoo') self.TestLint('static string foo;', - 'For a static/global string constant, use a C style ' - 'string instead: "static char foo[]".' - ' [runtime/string] [4]') + error_msg % 'static char foo') self.TestLint('static const string foo;', - 'For a static/global string constant, use a C style ' - 'string instead: "static const char foo[]".' - ' [runtime/string] [4]') + error_msg % 'static const char foo') self.TestLint('string Foo::bar;', - 'For a static/global string constant, use a C style ' - 'string instead: "char Foo::bar[]".' - ' [runtime/string] [4]') + error_msg % 'char Foo::bar') self.TestLint('string* pointer', '') self.TestLint('string *pointer', '') self.TestLint('string* pointer = Func();', '') @@ -2373,10 +2514,7 @@ class CpplintTest(CpplintTestBase): self.TestLint('string Foo::bar() {}', '') self.TestLint('string Foo::operator*() {}', '') # Rare case. - self.TestLint('string foo("foobar");', - 'For a static/global string constant, use a C style ' - 'string instead: "char foo[]".' - ' [runtime/string] [4]') + self.TestLint('string foo("foobar");', error_msg % 'char foo') # Should not catch local or member variables. self.TestLint(' string foo', '') # Should not catch functions. @@ -2400,6 +2538,34 @@ class CpplintTest(CpplintTestBase): ' return "";\n' '}\n', '') + # Check multiline cases. + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData('foo.cc', 'cc', + ['// Copyright 2014 Your Company.', + 'string Class', + '::MemberFunction1();', + 'string Class::', + 'MemberFunction2();', + 'string Class::', + 'NestedClass::MemberFunction3();', + 'string TemplateClass::', + 'NestedClass::MemberFunction4();', + 'string Class', + '::static_member_variable1;', + 'string Class::', + 'static_member_variable2;', + 'string Class', + '::static_member_variable3 = "initial value";', + 'string Class::', + 'static_member_variable4 = "initial value";', + ''], + error_collector) + self.assertEquals(error_collector.Results(), + [error_msg % 'char Class::static_member_variable1', + error_msg % 'char Class::static_member_variable2', + error_msg % 'char Class::static_member_variable3', + error_msg % 'char Class::static_member_variable4']) + def testNoSpacesInFunctionCalls(self): self.TestLint('TellStory(1, 3);', '') @@ -2571,7 +2737,7 @@ class CpplintTest(CpplintTestBase): # Test for NUL bytes only error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData('nul.cc', 'cc', - ['// Copyright 2008 Your Company.', + ['// Copyright 2014 Your Company.', '\0', ''], error_collector) self.assertEquals( error_collector.Results(), @@ -2582,7 +2748,7 @@ class CpplintTest(CpplintTestBase): error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'nul_utf8.cc', 'cc', - ['// Copyright 2008 Your Company.', + ['// Copyright 2014 Your Company.', unicode('\xe9x\0', 'utf8', 'replace'), ''], error_collector) self.assertEquals( @@ -2605,6 +2771,14 @@ class CpplintTest(CpplintTestBase): ['\n', '// {\n', '\n', '\n', '// Comment\n', '{\n', '}\n'], 0, 0) self.TestBlankLinesCheck(['\n', 'run("{");\n', '\n'], 0, 0) self.TestBlankLinesCheck(['\n', ' if (foo) { return 0; }\n', '\n'], 0, 0) + self.TestBlankLinesCheck( + ['int x(\n', ' int a) {\n', '\n', 'return 0;\n', '}'], 0, 0) + self.TestBlankLinesCheck( + ['int x(\n', ' int a) const {\n', '\n', 'return 0;\n', '}'], 0, 0) + self.TestBlankLinesCheck( + ['int x(\n', ' int a) {\n', '\n', 'return 0;\n', '}'], 1, 0) + self.TestBlankLinesCheck( + ['int x(\n', ' int a) {\n', '\n', 'return 0;\n', '}'], 1, 0) def testAllowBlankLineBeforeClosingNamespace(self): error_collector = ErrorCollector(self.assert_) @@ -2727,7 +2901,7 @@ class CpplintTest(CpplintTestBase): def testAllowBlankLinesInRawStrings(self): error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData('foo.cc', 'cc', - ['// Copyright 2008 Your Company.', + ['// Copyright 2014 Your Company.', 'static const char *kData[] = {R"(', '', ')", R"(', @@ -2885,6 +3059,9 @@ class CpplintTest(CpplintTestBase): self.TestLint('f(a, /* name */"1");', '') self.TestLint('f(1, /* empty macro arg */, 2)', '') self.TestLint('f(1,, 2)', '') + self.TestLint('operator,()', '') + self.TestLint('operator,(a,b)', + 'Missing space after , [whitespace/comma] [3]') def testIndent(self): self.TestLint('static int noindent;', '') @@ -2899,6 +3076,20 @@ class CpplintTest(CpplintTestBase): self.TestLint(' char* one_space_indent = "public:";', 'Weird number of spaces at line-start. ' 'Are you using a 2-space indent? [whitespace/indent] [3]') + self.TestLint(' public:', '') + self.TestLint(' protected:', '') + self.TestLint(' private:', '') + self.TestLint(' protected: \\', '') + self.TestLint(' public: \\', '') + self.TestLint(' private: \\', '') + self.TestMultiLineLint( + TrimExtraIndent(""" + class foo { + public slots: + void bar(); + };"""), + 'Weird number of spaces at line-start. ' + 'Are you using a 2-space indent? [whitespace/indent] [3]') self.TestMultiLineLint( TrimExtraIndent(''' static const char kRawString[] = R"(" @@ -3405,6 +3596,14 @@ class CpplintTest(CpplintTestBase): # Make sure we extracted something for our header guard. self.assertNotEqual(expected_guard, '') + # No header guard, but the error is suppressed. + error_collector = ErrorCollector(self.assert_) + cpplint.ProcessFileData(file_path, 'h', + ['// Copyright 2014 Your Company.', + '// NOLINT(build/header_guard)', ''], + error_collector) + self.assertEquals([], error_collector.ResultList()) + # Wrong guard error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData(file_path, 'h', @@ -3601,6 +3800,8 @@ class CpplintTest(CpplintTestBase): self.TestLint('#include "foo.h"', 'Include the directory when naming .h files' ' [build/include] [4]') + self.TestLint('#include "Python.h"', '') + self.TestLint('#include "lua.h"', '') def testBuildPrintfFormat(self): error_collector = ErrorCollector(self.assert_) @@ -3620,7 +3821,7 @@ class CpplintTest(CpplintTestBase): error_collector = ErrorCollector(self.assert_) cpplint.ProcessFileData( 'foo.cc', 'cc', - ['// Copyright 2008 Your Company.', + ['// Copyright 2014 Your Company.', r'printf("\\%%%d", value);', r'printf(R"(\[)");', r'printf(R"(\[%s)", R"(\])");', @@ -3731,7 +3932,7 @@ class CpplintTest(CpplintTestBase): 'You should have a line: "Copyright [year] "' ' [legal/copyright] [5]') - copyright_line = '// Copyright 2008 Google Inc. All Rights Reserved.' + copyright_line = '// Copyright 2014 Google Inc. All Rights Reserved.' file_path = 'mydir/googleclient/foo.cc' @@ -3930,14 +4131,10 @@ class CleansedLinesTest(unittest.TestCase): class OrderOfIncludesTest(CpplintTestBase): def setUp(self): + CpplintTestBase.setUp(self) self.include_state = cpplint._IncludeState() - # Cheat os.path.abspath called in FileInfo class. - self.os_path_abspath_orig = os.path.abspath os.path.abspath = lambda value: value - def tearDown(self): - os.path.abspath = self.os_path_abspath_orig - def testCheckNextIncludeOrder_OtherThenCpp(self): self.assertEqual('', self.include_state.CheckNextIncludeOrder( cpplint._OTHER_HEADER)) @@ -4154,6 +4351,11 @@ class OrderOfIncludesTest(CpplintTestBase): 'Should be: a.h, c system, c++ system, other. ' '[build/include_order] [4]')) + # Third party headers are exempt from order checks + self.TestLanguageRulesCheck('foo/foo.cc', + Format(['', '"Python.h"', '']), + '') + class CheckForFunctionLengthsTest(CpplintTestBase): @@ -4460,7 +4662,7 @@ class CheckForFunctionLengthsTest(CpplintTestBase): self.TestFunctionLengthsCheck( 'MACRO_WITH_UNDERSCORES(arg1, arg2, arg3)', '') - + self.TestFunctionLengthsCheck( 'NonMacro(arg)', 'Lint failed to find start of function body.' @@ -4942,7 +5144,6 @@ def tearDown(): if __name__ == '__main__': - import sys # We don't want to run the VerifyAllCategoriesAreSeen() test unless # we're running the full test suite: if we only run one test, # obviously we're not going to see all the error categories. So we