Summary: | Check for extra spacing on a variable declaration line. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Magnuson <smagnuso> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cjerdonek, hamaji, hausmann, manyoso, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Sam Magnuson
2010-06-16 11:20:37 PDT
Created attachment 58910 [details]
Proposed patch
Comment on attachment 58910 [details]
Proposed patch
Thanks for this patch! This feature is very useful, but I think we can improve this.
Two general comments:
- You may have already done this, but please check your patch with existing code and check if there are new false positives. For example, I often run ./WebKitTools/Scripts/run-webkit-style WebCore/*/*.cpp | grep <the new error message> to see the change.
- Please create a patch to the newest tree. I think cpp.py was moved from processors directory to checkers directory.
WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365
+ matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line)
This regexp will catch "else if" as well, and this is useful warning. So, I think we should change <type> and <name> to <token1> and <token2> or something like this.
WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365
+ matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line)
I think it would be nice if we can check "int* a", "int& a", "if (", etc.
WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1368
+ 'Declaration has space between %s and %s ' % (matched.group('type'), matched.group('name')) )
Having one space is OK so this sentence sounds a bit strange to me. Also, we don't need the trailing space.
So, I would say "Extra space between '%s' and '%s'" or something like this.
Created attachment 59016 [details]
Proposed patch
(In reply to comment #2) > (From update of attachment 58910 [details]) > Thanks for this patch! This feature is very useful, but I think we can improve this. > > Two general comments: > > - You may have already done this, but please check your patch with existing code and check if there are new false positives. For example, I often run ./WebKitTools/Scripts/run-webkit-style WebCore/*/*.cpp | grep <the new error message> to see the change. This seemed to pass for me, it even spotted a handful of existing errors :) > - Please create a patch to the newest tree. I think cpp.py was moved from processors directory to checkers directory. > > WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365 > + matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line) > This regexp will catch "else if" as well, and this is useful warning. So, I think we should change <type> and <name> to <token1> and <token2> or something like this. > Fair enough, I've done this - add added the 'else if' to the unittest. > WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1365 > + matched = search(r'^\s*(?P<type>\w+)\s\s+(?P<name>\w+)', line) > I think it would be nice if we can check "int* a", "int& a", "if (", etc. > Ok, I added this and put it in the unit test as well. > WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1368 > + 'Declaration has space between %s and %s ' % (matched.group('type'), matched.group('name')) ) > Having one space is OK so this sentence sounds a bit strange to me. Also, we don't need the trailing space. > > So, I would say "Extra space between '%s' and '%s'" or something like this. Agreed, changed. Comment on attachment 59016 [details] Proposed patch Looks great. Thanks for your quick update. > > - You may have already done this, but please check your patch with existing code and check if there are new false positives. For example, I often run ./WebKitTools/Scripts/run-webkit-style WebCore/*/*.cpp | grep <the new error message> to see the change. > > This seemed to pass for me, it even spotted a handful of existing errors :) Nice! Thanks for your checking. Created attachment 59437 [details]
Rediff against trunk
Attachment 59437 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitpy/style/checkers/cpp.py:1375: whitespace before ')' [pep8/E202] [5]
Total errors found: 1 in 3 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 59437 [details]
Rediff against trunk
Looks good. I'll land this with fixing the style error.
Committed r61660: <http://trac.webkit.org/changeset/61660> (In reply to comment #9) > Committed r61660: <http://trac.webkit.org/changeset/61660> It looks like the test doesn't success with your last patch because you forgot to modify the test expectations after you addressed the following comment. It's a very good idea to run WebKitTools/Scripts/test-webkit-scripts before you upload your patch when you modify our scripts. Anyway, thanks again for your contribution! > WebKitTools/Scripts/webkitpy/style/processors/cpp.py:1368 > + 'Declaration has space between %s and %s ' % (matched.group('type'), matched.group('name')) ) > Having one space is OK so this sentence sounds a bit strange to me. Also, we don't need the trailing space. > > So, I would say "Extra space between '%s' and '%s'" or something like this. |