[Webkit-unassigned] [Bug 32487] [check-webkit-style] cpp_style.py contains code redundant with check-webkit-style

[bugzilla-daemon at webkit.org]

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


David Levin <levin at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #44759|review?                     |review-
               Flag|                            |




--- Comment #7 from David Levin <levin at chromium.org>  2009-12-15 15:57:05 PST ---
(From update of attachment 44759)
Just a few things to consider addressing below. Thanks!

> Index: ChangeLog
> +2009-12-13  Chris Jerdonek  <chris.jerdonek at gmail.com>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Removed obsolete code in cpp_style.py and other clean-up.
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=32487
> +
> +        Eliminated obsolete main() and _USAGE code from cpp_style.py.
> +        Added default flag values to check-webkit-style's --help output.
> +        Other minor, related code clean-ups.
> +
> +        * Scripts/check-webkit-style:
> +        * Scripts/modules/cpp_style.py:
> +        * Scripts/modules/cpp_style_unittest.py:

Just fyi, per file comments are more desirable than the overall summary
comment.

> Index: Scripts/check-webkit-style
> +      The output format, which can be one of--

s/can/may/ and no --


>  
>  def process_patch(patch_string):
> @@ -100,32 +125,34 @@ def process_patch(patch_string):
>      for filename, diff in patch.files.iteritems():
>          file_extension = os.path.splitext(filename)[1]
>  
> -        if file_extension in ['.cpp', '.c', '.h']:
> -            line_numbers = set()
> +        if file_extension not in ['.cpp', '.c', '.h']:
> +            continue
> +
> +        line_numbers = set()
>  
> -            def error_for_patch(filename, line_number, category, confidence, message):
> -                """Wrapper function of cpp_style.error for patches.
> +        def error_for_patch(filename, line_number, category, confidence, message):
> +            """Wrapper function of cpp_style.error for patches.
>  
> -                This function outputs errors only if the line number
> -                corresponds to lines which are modified or added.
> -                """
> -                if not line_numbers:
> -                    for line in diff.lines:
> -                        # When deleted line is not set, it means that
> -                        # the line is newly added.
> -                        if not line[0]:
> -                            line_numbers.add(line[1])
> +            This function outputs errors only if the line number
> +            corresponds to lines which are modified or added.
> +            """
> +            if not line_numbers:
> +                for line in diff.lines:
> +                    # When deleted line is not set, it means that
> +                    # the line is newly added.
> +                    if not line[0]:
> +                        line_numbers.add(line[1])
>  
> -                if line_number in line_numbers:
> -                    cpp_style.error(filename, line_number, category, confidence, message)
> +            if line_number in line_numbers:
> +                cpp_style.error(filename, line_number, category, confidence, message)
>  
> -            cpp_style.process_file(filename, error=error_for_patch)
> +        cpp_style.process_file(filename, error=error_for_patch)

I don't see how this part of the change has to do with your bug "cpp_style.py
contains code redundant with check-webkit-style"

Please do it separately (and then the changelog will indicate what you are
trying to accomplish here .. at first glance, it looks like it changes the
logic).



> +    (files, flags) = cpp_style.parse_arguments(sys.argv[1:], ["git-commit="], True)

It would be good to specify a parameter name for the True (even if this
parameter will go away after a patch later).


> +        cpp_style.exit_with_usage('It is not possible to check files and a '
> +                                  'specific commit at the same time.', True)

It would be nice to specify the parameter name for the True here.


> Index: Scripts/modules/cpp_style_unittest.py

> -    # 
> +    #
In general, please don't do end of line whitespace clean up. It isn't a webkit
style thing and it makes big code reviews even bigger.


> -    # 
> +    #

Ditto.

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