I've been bitten before by having: struct { int a; int b; } As my previous guidelines had me working that way. I've had reviews ask that I fix this, and it would have been great to have check-webkit-style flag this for me to prevent wasting reviewer time.
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.