[Webkit-unassigned] [Bug 27291] Support lint for patches

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=27291





--- Comment #6 from David Levin <levin at chromium.org>  2009-07-21 00:56:54 PDT ---
(From update of attachment 33148)
I saw that you unmarked this for review but I suspect these comments apply to
what you are about to submit.  btw, I agree completely with making the git
commit argument --git-commit=<committish> and was about to suggest it simply
because that is consistent with prepare-ChangeLog.


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

The changes done here look good but I think you missed the call in def main()
to process_file(filename, _cpplint_state.verbose_level)
It should be easy enough to set the verbose level and omit the second
parameter.


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

> +# Override the usage of the lint tool.
> +cpplint._USAGE = """
> +Syntax: webkit-run-lint [--verbose=#] [--output=vs7] [--filter=-x,+y,...]
> +        [COMMITISH] ...
> +
> +  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.
> +
> +  If you don't specify COMMITISH, the current change on your repository will
> +  be linted.  Otherwise, the specified commits will be linted based on your
> +  local files.  Note that if you specify old commits which are modified in
> +  newer patches, the line number of the lint results may be wrong.
> +  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=
> +"""

Consider doing this

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


> +def process_patch(patch_string):
> +    """Does lint on a single patch.
> +
> +    Args:
> +      patch_string: A string of a patch.
> +    """
> +    patch = DiffParser(patch_string.splitlines())
> +    for filename, diff in patch.files.iteritems():
> +        file_extension = os.path.splitext(filename)[1]
> +
> +        if file_extension in ['.cc', '.cpp', '.h']:
> +            line_numbers = set()

Consider creating this on demand, so if there are no lint errors, it just
doesn't get created.  Like this:

               line_numbers = None            
> +
> +            def error_for_patch(filename, line_number, category, confidence, message):
> +                """Wrapper function of cpplint.error for patches.
> +
> +                This function outputs errors only if the line number
> +                corresponds to lines which are modified or added.
> +                """
                   if line_numbers is None:
                       line_numbers = set()
                       for line in diff.lines:
                           line_numbers.add(line[1])

> +                if line_number in line_numbers:
> +                    cpplint.error(filename, line_number, category, confidence, message)
> +
> +            cpplint.process_file(filename, error=error_for_patch)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.