Bug 34932 - check-webkit-style: Code clean-up prior to dividing ProcessorOptions into two classes
Summary: check-webkit-style: Code clean-up prior to dividing ProcessorOptions into two...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chris Jerdonek
URL:
Keywords:
Depends on:
Blocks: 34674
  Show dependency treegraph
 
Reported: 2010-02-15 00:46 PST by Chris Jerdonek
Modified: 2010-02-16 23:41 PST (History)
2 users (show)

See Also:


Attachments
Proposed patch (36.16 KB, patch)
2010-02-16 14:07 PST, Chris Jerdonek
no flags Details | Formatted Diff | Diff
Proposed patch 2 (36.11 KB, patch)
2010-02-16 14:13 PST, Chris Jerdonek
hamaji: 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 2010-02-15 00:46:07 PST
Splitting the patch of the below report into two, as requested:

https://bugs.webkit.org/show_bug.cgi?id=34674#c2
Comment 1 Chris Jerdonek 2010-02-16 14:07:03 PST
Created attachment 48829 [details]
Proposed patch

Note that I removed these patterns from _PATH_RULES_SPECIFIER:

-    # These are test file patterns.
-    (["_test.cpp",
-      "_unittest.cpp",
-      "_regtest.cpp"],
-     ("-readability/streams",  # Many unit tests use cout.
-      "-runtime/rtti")),

I found that no files in WebKit match them when looking for actual paths for the _PATH_RULES_SPECIFIER unit tests.  I recall Adam saying in a bugs.webkit.org comment that these were used previously by Google.  Thanks!
Comment 2 Chris Jerdonek 2010-02-16 14:13:44 PST
Created attachment 48830 [details]
Proposed patch 2

Minor correction to ChangeLog.
Comment 3 Shinichiro Hamaji 2010-02-16 23:00:58 PST
Comment on attachment 48830 [details]
Proposed patch 2

Looks good. Thanks a lot for isolating this change!

> +# Each string appearing in this nested list should have at least
> +# one associated unit test assertion.

I'd say where we should add the test. Maybe "See test_path_rules_specifier in checker_unittest.py" or something like this.
Comment 4 Chris Jerdonek 2010-02-16 23:20:39 PST
(In reply to comment #3)
> (From update of attachment 48830 [details])
> Looks good. Thanks a lot for isolating this change!

Sure thing.  Thanks for your quick review!

> > +# Each string appearing in this nested list should have at least
> > +# one associated unit test assertion.
> 
> I'd say where we should add the test. Maybe "See test_path_rules_specifier in
> checker_unittest.py" or something like this.

I was going to do this, but I decided against because comments like these can fall out of synch more easily.  I will add it back.  Thanks for the feedback.
Comment 5 Chris Jerdonek 2010-02-16 23:21:40 PST
Comment on attachment 48830 [details]
Proposed patch 2

cq- to commit manually.
Comment 6 Chris Jerdonek 2010-02-16 23:41:35 PST
Manually committed:

http://trac.webkit.org/changeset/54874