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

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

The file cpp_style.py contains some chunks of code that seem obsolete and superseded by what is now contained in check-webkit-style.

This is a place-holder report for this and some related code clean-up.  Patch forthcoming.

[Chris Jerdonek]

Created attachment 44752 [details]
Proposed patch

Some of the changes:

- Eliminated obsolete main() and _USAGE from cpp_style.py.
- Moved cpp_style.py's file documentation to check-webkit-style.
- Added default values to _USAGE string in check-webkit-style.
- Added the default filter values to the --filter= output.
- Made style categories a list rather than a string.
- Moved _WEBKIT_FILTER to be near the other global constants.
- Renamed _DEFAULT_FILTERS to be consistent with --filter and _WEBKIT_FILTER.
- Renamed two "print_" methods to "exit_with_" since they exit.
- Cleaned up other minor issues.

[Chris Jerdonek]

Created attachment 44759 [details]
Proposed patch 2

Tweaked previous patch:

- Changed double quotes to single quotes in _STYLE_CATEGORIES.
- Also changed global "FILTER" references to "FILTER_RULES".
  This is because there was confusion in using "filter" to refer 
  both to an individual filter rule (e.g. "+foo") as well as to
  the filter resulting from a list of such rules (a filter 
  being a list of filters).  Instead I chose to use "filter" for 
  the collective rule and "filter rule" for the individual 
  boolean rules making up a filter.

[Eric Seidel (no email)]

So this undoubtably comes from the fact that cpp_style.py is standalone in its original Chromium/Google form.  But I think we should continue to diverge and make improvements, so this is good.  Dave Levin is probably your best reviewer though.

[David Levin]

Yeah, I glanced at it earlier, and I didn't see anything terrible :) but I needed to finish up https://bugs.webkit.org/show_bug.cgi?id=32214 which was involved. I'll look tomorrow.

[Shinichiro Hamaji]

Comment on attachment 44759 [details]
Proposed patch 2

Though I'm not a reviewer, I'd like to put some comments.

> +"""Does WebKit-lint on c++ files.

"on C/C++ files" would be better.

> -        if file_extension in ['.cpp', '.c', '.h']:
> -            line_numbers = set()
> +        if file_extension not in ['.cpp', '.c', '.h']:
> +            continue
> +
> +        line_numbers = set()

This refactoring is nice in general, but this will conflict with the patch in Bug 32538. Please avoid doing this.

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

I prefer display_help=True to True as the meaning becomes clearer in this way.

> -        sys.stderr.write("ERROR: It is not possible to check files "
> -                          "and a specific commit at the same time.\n" + cpp_style._USAGE)
> -        sys.exit(1)
> +        cpp_style.exit_with_usage('It is not possible to check files and a '
> +                                  'specific commit at the same time.', True)

Same as above. Please consider using display_help=True.

[Chris Jerdonek]

(In reply to comment #5)
> (From update of attachment 44759 [details])
> Though I'm not a reviewer, I'd like to put some comments.
> 
> > +"""Does WebKit-lint on c++ files.
> 
> "on C/C++ files" would be better.

Note that I simply moved the existing string that was in cpp_style.py to check-webkit-style.  I can change this though.

> >  def main():
> > -    cpp_style.use_webkit_styles()
> > +    cpp_style._DEFAULT_FILTER_RULES = cpp_style._WEBKIT_FILTER_RULES
> >  
> > -    (files, flags) = cpp_style.parse_arguments(sys.argv[1:], ["git-commit="])
> > +    (files, flags) = cpp_style.parse_arguments(sys.argv[1:], ["git-commit="], True)
> 
> I prefer display_help=True to True as the meaning becomes clearer in this way.

Note that this is only meant to be temporary.  I chose display_help=false for now because there are many calls to parse_arguments in the test_parse_arguments unit test that require the display to be suppressed.  I was planning to refactor parse_arguments and its unit tests in my next patch for other reasons.  That patch will eliminate the need for a display_help parameter entirely.

[David Levin]

Comment on attachment 44759 [details]
Proposed patch 2

Just a few things to consider addressing below. Thanks!

> Index: ChangeLog
> [phone]  Chris Jerdonek  <[e-mail]>
> +
> +        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.

[Chris Jerdonek]

(In reply to comment #6)

> > I prefer display_help=True to True as the meaning becomes clearer in this way.
> 
> Note that this is only meant to be temporary.  I chose display_help=false for

Shinichiro, I misunderstood your comment on this before so you can disregard my response to it.  I thought you meant that you preferred the default values for these functions to be display_help=True.  But now I see you meant the same as what Dave suggested -- to call with the keyword syntax.  Thanks.

[Chris Jerdonek]

Created attachment 44935 [details]
Proposed patch 3

- Incorporated all of Dave and Shinichiro's comments.  This includes
  removing the addition of the guard clause to process_patch.
- Also added a code comment explaining the decision to use the "filter
  rule" terminology rather than "filter" for an individual boolean 
  filter item.

Thanks, guys, for taking the time to review this larger patch so quickly.

[WebKit Commit Bot]

Comment on attachment 44935 [details]
Proposed patch 3

Rejecting patch 44935 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'David Levin', '--force']" exit_code: 1
Last 500 characters of output:
tch.
9 out of 9 hunks ignored
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: Scripts/modules/cpp_style_unittest.py
|===================================================================
|--- Scripts/modules/cpp_style_unittest.py	(revision 52066)
|+++ Scripts/modules/cpp_style_unittest.py	(working copy)
--------------------------
No file to patch.  Skipping patch.
7 out of 7 hunks ignored

Full output: http://webkit-commit-queue.appspot.com/results/128406

[Chris Jerdonek]

(In reply to comment #10)
> 
> Failed to run
> "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply',
> '--reviewer', 'David Levin', '--force']" exit_code: 1
> Last 500 characters of output:
> tch.
> 9 out of 9 hunks ignored
> can't find file to patch at input line 5
> Perhaps you used the wrong -p or --strip option?
> The text leading up to this was:
> --------------------------
> |Index: Scripts/modules/cpp_style_unittest.py
> |===================================================================
> |--- Scripts/modules/cpp_style_unittest.py    (revision 52066)
> |+++ Scripts/modules/cpp_style_unittest.py    (working copy)
> --------------------------

It looks like this failed because the file paths are missing the WebKitTools prefix.  Did I do something wrong?

For this change I only checked out the WebKitTools folder, using a command like--

svn checkout http://svn.webkit.org/repository/webkit/trunk/WebKitTools Tools1

(I also renamed Tools1 to something else after that.)  I ran svn-create-patch from within that folder to create the patch.  Let me know the best way to correct this.  I assume there is a convenient way to land patches that doesn't require checking out the entire tree again?  (Generally, I would not mind receiving advice on how to best manage working on multiple, possibly overlapping changes at once.)  Thanks.

[Kent Tamura]

I landed this patch manually:
http://trac.webkit.org/changeset/52189

[Eric Seidel (no email)]

The commit-queue only works with patches which are made from the root directory.  Our tools all will do that automatically for you if you have a full checkout.  In general I would say that partial checkouts are not a supported development environment for webkit.org