[webkit-reviews] review denied: [Bug 27291] Support lint for patches : [Attachment 33158] Patch v3
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jul 21 02:39:10 PDT 2009
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)
More information about the webkit-reviews
mailing list