[webkit-reviews] review denied: [Bug 32487] [check-webkit-style] cpp_style.py contains code redundant with check-webkit-style : [Attachment 44759] Proposed patch 2

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Dec 15 15:57:04 PST 2009


David Levin <levin at chromium.org> has denied Chris Jerdonek
<chris.jerdonek at gmail.com>'s request for review:
Bug 32487: [check-webkit-style] cpp_style.py contains code redundant with
check-webkit-style
https://bugs.webkit.org/show_bug.cgi?id=32487

Attachment 44759: Proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=44759&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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.


More information about the webkit-reviews mailing list