From a8ee7eaaa4e4671c157ae8cba146a2a62d9ddc05 Mon Sep 17 00:00:00 2001 From: "avakulenko@google.com" Date: Mon, 11 Aug 2014 19:41:35 +0000 Subject: [PATCH] Update cpplint.py to #387: 387 - Allow lint warnings on the next line to be suppressed with NOLINTNEXTLINE. 386 - Fixed false positive for strcat and strcpy. 385 - Fixed false positive for bitwise and on last line of a multi-line 384 - Remove unused deprecated testing/base/googletest.h header. 383 - Fixed false positive for bitwise and on last line of a multi-line initializer list that ends with closing brace on the same line. 382 - Allow header guard warning to be silenced with "// NOLINT(build/header_guard)" comment. 381 - Removed check for spaces between "//" and "TODO". 380 - Fix typo: Redudnant -> Redundant 379 - Added check for redundant "virtual" and "override" function-specifier. 378 - Updated missing "explicit" message to match style guide wording 377 - Add cpplint check to prevent .cc file inclusions from other packages. These are generally bad and we should prevent regressions like this until proper BUILD layering checks are in place. 376 - Silence missing braces warning for conditional statements inside lambdas. 375 - Warn about multi-arg constructors being explicit only at highest severity level. 374 - Warn about non-single-argument constructors marked explicit. 373 - Exclude 'case' from function spacing check. 372 - Exclude third party headers such as Python.h from these checks: 371 - Fixed false positive for function pointer parameters followed by other parameters. 370 - Allow RValue reference for default inline constructor. 369 - Fixed false positive for array of references and operator,(). 368 - Silence named parameter warning for macros. 367 - Fixed false positive for duplicate header warning across preprocessor boundaries. 366 - Allow rvalue references for constructors and assignment operator. 365 - Stop handling DISALLOW_EVIL_CONSTRUCTORS in cpplint.py, since it's going away. 364 - Fixed false positive for member function in nested classes. 363 - Namespace indentation for cpplint. The C++ style guide enforced that items within a namespace do not receive an extra level of indentation. 362 - Fixed false positive for multi-line member functions returning strings. 361 - Do not suggest replacement CHECK macros if custom error messages are used. 360 - Silence nonconst reference warning for multiline derived functions. 359 - Silence non-const reference warnings for derived functions. 358 - Added link to developer guide. 357 - Remove the partial ban on std::move and related features. More general 356 - Fixed false positive for << inside macros. 355 - Make _NestingState class public. 354 - Fixed false positive for multiline function templates. 353 - Fixed false positive for lambda capture. 352 - Silence RValue reference warnings that are enclosed in a 351 - Do not warn on CR-LF lines if all input lines are uniformly CR-LF. 350 - Tweaked the error messages for static mutex with default constructors. 349 - Fixed false positive for unnamed parameters in macros. 348 - Recognize &&... as RValue references. 347 - Use alternative error message for including . 346 - Fixed false positive for function style template argument. 345 - Fixed false positive for braced constructor call inside brackets. 344 - Minor spelling and grammar fix. 343 - Fixed false positive for non-const reference check inside constructor 342 - Fixed cases where rvalue references are not identified correctly: 341 - Check that third party header files have some header guard, but do not 340 - Don't check header guards in third party files. 339 - Instead of treating thread annotations as macro function calls, skip over them in parsing function signatures. This silences false positives for lambdas with thread annotations. 338 - Fixed false positive for deprecated cast where return type of function 337 - Fixed false positive for alias-declarations of function pointers. 336 - Improved error message for taking address of something dereferenced 335 - Added support for C++14 digit separators. 334 - Silence long line warnings for JNI. 333 - Fixed false positive for constructor calls for rand{} being 332 - Elide C++11 raw strings as an empty quoted string instead of 331 - Split up long functions in cpplint.py and cpplint_unittest.py 330 - Fixed false positive for missing space check around "using operator;" 329 - Fixed false positive for indent check around multi-line raw strings. R=erg@google.com Review URL: https://codereview.appspot.com/125980043 --- cpplint/cpplint.py | 554 +++++++++++++++++++---- cpplint/cpplint_unittest.py | 879 ++++++++++++++++++++++-------------- 2 files changed, 1004 insertions(+), 429 deletions(-) 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