[webkit-reviews] review denied: [Bug 27291] Support lint for patches : [Attachment 33158] Patch v3

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Shinichiro Hamaji
<hamaji at chromium.org>'s request for review:
Bug 27291: Support lint for patches
https://bugs.webkit.org/show_bug.cgi?id=27291

Attachment 33158: Patch v3
https://bugs.webkit.org/attachment.cgi?id=33158&action=review

------- Additional Comments from David Levin <levin at chromium.org>
A few things to consider....

> diff --git a/WebKitTools/Scripts/modules/cpplint.py
b/WebKitTools/Scripts/modules/cpplint.py

> +# FIXME: Once run-webkit-lint script becomes ready, we don't need to
> +#	    support standalone execution for this module.
> +#	    So, we should remove _USAGE and main() eventually.

I'm not sure this is necessary (maybe).


>      _cpplint_state.reset_error_count()

Add this line here:

   _set_verbose_level(_cpplint_state.verbose_level)

>      for filename in filenames:
> -	   process_file(filename, _cpplint_state.verbose_level)
> +	   process_file(filename)


> diff --git a/WebKitTools/Scripts/run-webkit-lint
b/WebKitTools/Scripts/run-webkit-lint

> +cpplint._USAGE = """
> +Syntax: webkit-run-lint [--verbose=#] [--output=vs7] [--filter=-x,+y,...]
> +
> +  The style guidelines this tries to follow are those in
> +    http://webkit.org/coding/coding-style.html
> +
> +  Every problem is given a confidence score from 1-5, with 5 meaning we are
> +  certain of the problem, and 1 meaning it could be a legitimate construct.
> +  This will miss some errors, and is not a substitute for a code review.
> +
> +  To prevent specific lines from being linted, add a '// NOLINT' comment to
the
> +  end of the line.
> +
> +  Linted extensions are .cc, .cpp, and .h.  Other file types will be
ignored.
> +
> +  Flags:
> +
> +    output=vs7
> +	 By default, the output is formatted to ease emacs parsing.  Visual
Studio
> +	 compatible output (vs7) may also be used.  Other formats are
unsupported.
> +
> +    verbose=#
> +	 Specify a number 0-5 to restrict errors to certain verbosity levels.
> +
> +    filter=-x,+y,...
> +	 Specify a comma-separated list of category-filters to apply: only
> +	 error messages whose category names pass the filters will be printed.
> +	 (Category names are printed with the message and look like
> +	 "[whitespace/indent]".)  Filters are evaluated left to right.
> +	 "-FOO" and "FOO" means "do not print categories that start with FOO".
> +	 "+FOO" means "do print categories that start with FOO".
> +
> +	 Examples: --filter=-whitespace,+whitespace/braces
> +		   --filter=whitespace,runtime/printf,+runtime/printf_format
> +		   --filter=-,+build/include_what_you_use
> +
> +	 To see a list of all the categories used in cpplint, pass no arg:
> +	    --filter=
> +"""

When I said this:  Consider doing this

""" ....""" % {'program_name' sys.argv[0]}
In the description text replace "webkit-run-lint" and "cpplint" with
"%{program_name}s"

I was referring to this comment (not the one in cpplint.py).  It would fix two
things: "webkit-run-lint" is not the program name. "cpplint" is also not the
name of the program being run.
sys.argv[0] is the program name.

>
> +	   if file_extension in ['.cc', '.cpp', '.h']:
> +	       line_numbers = set()
This looks fine.  The problem with my suggestion was the assigment inside of
error_for_patch which made the variable have scope local to that block.


> +    if args:
> +	   sys.stderr.write('ERROR: We don\'t support files as arguments for
now.\n' + cpplint._USAGE)

Consider using "" for the outside quotes and change \' to '.  Like this

	sys.stderr.write("ERROR: We don't support files as arguments for
now.\n" + cpplint._USAGE)