Bug 32592 - [check-webkit-style] Preparatory code moves to refactor cpp_style.parse_arguments
Summary: [check-webkit-style] Preparatory code moves to refactor cpp_style.parse_argum...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P4 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks: 32966
  Show dependency treegraph
 
Reported: 2009-12-15 19:44 PST by Chris Jerdonek
Modified: 2009-12-27 11:55 PST (History)
7 users (show)

See Also:


Attachments
Proposed patch (46.80 KB, patch)
2009-12-19 00:45 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 2 (46.76 KB, patch)
2009-12-20 12:57 PST, Chris Jerdonek
cjerdonek: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 3 (dividing earlier patch into two) (35.26 KB, patch)
2009-12-21 01:05 PST, Chris Jerdonek
cjerdonek: commit-queue-
Details | Formatted Diff | Diff
Proposed patch 4 (35.25 KB, patch)
2009-12-21 01:40 PST, Chris Jerdonek
levin: review+
cjerdonek: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Jerdonek 2009-12-15 19:44:01 PST
The function cpp_style.parse_arguments currently has side effects of setting global variables (and also writing to sys.stderr).

Refactor parse_arguments so this doesn't happen.  This will make unit testing easier and also make it more transparent when called from check-webkit-style.

I am working on this.
Comment 1 Chris Jerdonek 2009-12-19 00:45:11 PST
Created attachment 45227 [details]
Proposed patch

Let me know if you want me to split this into two patches.

It looks longer than it really is because of some large deletes.

In any case, I just want to get it out there so you can see it in one piece.  Thanks.
Comment 2 Chris Jerdonek 2009-12-20 12:57:37 PST
Created attachment 45290 [details]
Proposed patch 2

Brought up to date.
Comment 3 Shinichiro Hamaji 2009-12-20 22:39:00 PST
Comment on attachment 45290 [details]
Proposed patch 2

> Let me know if you want me to split this into two patches.
>
> It looks longer than it really is because of some large deletes.

Yeah, I think it would be better to split this patch. I'd suggest moving code chunk first, then refactoring.

> @@ -360,15 +310,15 @@ class _CppStyleState(object):
>      """Maintains module-wide state.."""
>  
>      def __init__(self):
> -        self.verbose_level = _DEFAULT_VERBOSITY  # global setting.
> +        self.verbose_level = 1  # global setting.
>          self.error_count = 0    # global count of reported errors
>          # filters to apply when emitting error messages
> -        self.filters = _DEFAULT_FILTER_RULES[:]
> +        self.filters = []
>  
>          # output format:
>          # "emacs" - format that emacs can parse (default)
>          # "vs7" - format that Microsoft Visual Studio 7 can parse
> -        self.output_format = _DEFAULT_OUTPUT_FORMAT
> +        self.output_format = 'emacs'
>  
>      def set_output_format(self, output_format):
>          """Sets the output format for errors."""

I'm not sure, but the default value of verbose_level, filters, and output_format must be always overwritten? If so, None would be better as the initial value? If you are planning to remove _CppStyleState, it's OK as is.
Comment 4 Chris Jerdonek 2009-12-20 23:08:29 PST
(In reply to comment #3)

> Yeah, I think it would be better to split this patch. I'd suggest moving code
> chunk first, then refactoring.

Thanks for the comments, Shinichiro.  Sure, I'll go ahead and do that.

> > @@ -360,15 +310,15 @@ class _CppStyleState(object):
> > -        self.filters = _DEFAULT_FILTER_RULES[:]
> > +        self.filters = []
> I'm not sure, but the default value of verbose_level, filters, and
> output_format must be always overwritten? If so, None would be better as the
> initial value? If you are planning to remove _CppStyleState, it's OK as is.

Yes, I was planning to remove those properties in a future patch.  I just didn't want it to look like the global variables were playing a role there.
Comment 5 Chris Jerdonek 2009-12-21 01:05:02 PST
Created attachment 45313 [details]
Proposed patch 3 (dividing earlier patch into two)

Dividing into two patches as Shinichiro requested.  This patch has all of the moves with the minimum changes needed for things to work.
Comment 6 Chris Jerdonek 2009-12-21 01:40:27 PST
Created attachment 45317 [details]
Proposed patch 4

I noticed that the defaults were not getting set after the code move, so I changed two lines of style.parse_arguments() to this:

    verbosity = _DEFAULT_VERBOSITY
    output_format = _DEFAULT_OUTPUT_FORMAT

I'm not sure how much this matters since it will be refactored away momentarily.
Comment 7 Shinichiro Hamaji 2009-12-21 01:43:22 PST
Comment on attachment 45313 [details]
Proposed patch 3 (dividing earlier patch into two)

Thanks for splitting the patch! Looks good assuming this patch is basically moving code.

> @@ -48,117 +47,9 @@ import sys
>  import unicodedata
>  
>  
> -# This is set by check-webkit-style.
>  _USAGE = ''

We don't need this in cpp_style anymore?
Comment 8 Chris Jerdonek 2009-12-21 01:52:21 PST
(In reply to comment #7)
> (From update of attachment 45313 [details])
> Thanks for splitting the patch! Looks good assuming this patch is basically
> moving code.

Yeah, pretty much.  The only changes within moves were selectively changing some "cpp_style." prefixes to "style." or "".

> >  _USAGE = ''
> We don't need this in cpp_style anymore?

You're right -- we don't.  I can get rid of it in part two if that's okay.
Comment 9 David Levin 2009-12-22 15:24:56 PST
Comment on attachment 45313 [details]
Proposed patch 3 (dividing earlier patch into two)

It appears that patch 4 replaces this one.
Comment 10 Chris Jerdonek 2009-12-22 16:21:26 PST
(In reply to comment #9)
> (From update of attachment 45313 [details])
> It appears that patch 4 replaces this one.

Thanks, David.  Does Bugzilla allow us to "obsolete" a patch without attaching a new one?  I couldn't find where to do this in the UI after forgetting to check the obsolete box.
Comment 11 Eric Seidel (no email) 2009-12-22 16:24:03 PST
You can click on the "details" for a patch to obsolete it.

If you have lots of patches to obsolete, then "bugzilla-tool obsolete-attachments BUGNUM" will obsolete all attachments on a bug for you.
Comment 12 David Levin 2009-12-22 16:26:28 PST
Eric said it. I should finish one this sometime today.
Comment 13 David Levin 2009-12-23 13:08:24 PST
Comment on attachment 45317 [details]
Proposed patch 4

> Index: ChangeLog

WebKitTools/ChangeLog....
This isn't going to land easily (using cq+) since the patch isn't rooted at WebKit.


> Index: Scripts/modules/cpp_style.py
> -# This is set by check-webkit-style.
>  _USAGE = ''

Is this _USAGE still needed?
Comment 14 Chris Jerdonek 2009-12-23 19:31:01 PST
(In reply to comment #13)

Thanks for the review, David.

> > Index: Scripts/modules/cpp_style.py
> > -# This is set by check-webkit-style.
> >  _USAGE = ''
> 
> Is this _USAGE still needed?

No, I will remove it in the second patch.
Comment 15 Shinichiro Hamaji 2009-12-23 21:31:47 PST
Committed r52541: <http://trac.webkit.org/changeset/52541>
Comment 16 Shinichiro Hamaji 2009-12-23 21:46:14 PST
Comment on attachment 45317 [details]
Proposed patch 4

It seems the ChangeLog had two issues:

- It didn't have "Reviewed by NOBODY (OOPS!)". Please consider using prepare-ChangeLog to add a ChangeLog entry.
- It had only two lines of context and patch command was confused. Normally, it should have 3 lines in the context. This also happened when I landed another patch you posted. I'm not sure why this happened.
Comment 17 Chris Jerdonek 2009-12-24 01:31:18 PST
(In reply to comment #16)
> (From update of attachment 45317 [details])
> It seems the ChangeLog had two issues:

Thank you for landing this patch, Shinichiro, and for correcting and reporting on the ChangeLog issues.

I do use prepare-Changelog.  I must have overwritten the "Reviewed by" line by accident.  I will be more attentive to that in the future.

I don't know why the context has only two lines at the end of that portion.  I tried re-running svn-create-patch and verified that it has only two lines.  In contrast, running "svn diff" does generate three lines of context at the end.  I will investigate further for a possible cause and, if necessary, file a bug report.  Thanks.
Comment 18 Chris Jerdonek 2009-12-24 09:47:25 PST
(In reply to comment #17)
> I will investigate further for a possible cause and, if necessary, file a bug
> report.  Thanks.

I created a bug report for this issue here:

https://bugs.webkit.org/show_bug.cgi?id=32919
Comment 19 Chris Jerdonek 2009-12-26 21:23:00 PST
(In reply to comment #15)
> Committed r52541: <http://trac.webkit.org/changeset/52541>

It looks like the commit did not include the following new file in the patch: Scripts/modules/style_unittest.py.  Also, the run-webkit-unittests scripts can't execute without this file.
Comment 20 Daniel Bates 2009-12-27 11:29:31 PST
Added missing file style_unittest.py and committed in <http://trac.webkit.org/changeset/52583>. Without this file, run-webkit-unittests died with the following error:

Traceback (most recent call last):
  File "WebKitTools/Scripts/run-webkit-unittests", line 49, in <module>
    from modules.style_unittest import *
ImportError: No module named style_unittest


(In reply to comment #19)
> (In reply to comment #15)
> > Committed r52541: <http://trac.webkit.org/changeset/52541>
> 
> It looks like the commit did not include the following new file in the patch:
> Scripts/modules/style_unittest.py.  Also, the run-webkit-unittests scripts
> can't execute without this file.
Comment 21 Chris Jerdonek 2009-12-27 11:55:52 PST
Moving part 2 to here:

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