Trim more unneeded stuff from Blink's C++ style checker script. (issue 2384943002 by dcheng@chromium.org)

1 view
Skip to first unread message

dch...@chromium.org

unread,
Oct 1, 2016, 3:48:38 AM10/1/16
to esp...@chromium.org, tha...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org
Reviewers: esprehn, Nico (mostly away until Oct 3)
CL: https://codereview.chromium.org/2384943002/

Message:
For whoever sees this first =)

Trims check-webkit-style running time across most (all?) of Blink source from
5m18s to 4m33s or so on my vm instance.

Description:
Trim more unneeded stuff from Blink's C++ style checker script.

This formatting is enforced by the clang-format check, so there's no
need to try to enforce it with regex that try to parse C++.

BUG=none

Affected files (+5, -800 lines):
M third_party/WebKit/Tools/Scripts/webkitpy/style/checker.py
M third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
M third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp_unittest.py


tha...@chromium.org

unread,
Oct 1, 2016, 12:39:46 PM10/1/16
to dcheng...@chromium.org, esp...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org
lgtm


https://codereview.chromium.org/2384943002/diff/40001/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
File third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
(right):

https://codereview.chromium.org/2384943002/diff/40001/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py#newcode1790
third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py:1790:
'Semicolon defining empty statement for this loop. Use { } instead.')
I think this is caught by a compiler warning too

https://codereview.chromium.org/2384943002/

dch...@chromium.org

unread,
Oct 1, 2016, 3:48:04 PM10/1/16
to esp...@chromium.org, tha...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org

https://codereview.chromium.org/2384943002/diff/40001/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
File third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
(right):

https://codereview.chromium.org/2384943002/diff/40001/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py#newcode1790
third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py:1790:
'Semicolon defining empty statement for this loop. Use { } instead.')
On 2016/10/01 16:39:46, Nico (mostly away until Oct 3) wrote:
> I think this is caught by a compiler warning too

At least in a simple test, I wasn't able to get this warning to fire.

https://codereview.chromium.org/2384943002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 3:49:00 PM10/1/16
to dcheng...@chromium.org, esp...@chromium.org, tha...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 4:34:13 PM10/1/16
to dcheng...@chromium.org, esp...@chromium.org, tha...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org
Committed patchset #3 (id:40001)

https://codereview.chromium.org/2384943002/

commit-bot@chromium.org via codereview.chromium.org

unread,
Oct 1, 2016, 4:35:53 PM10/1/16
to dcheng...@chromium.org, esp...@chromium.org, tha...@chromium.org, commi...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org
Patchset 3 (id:??) landed as
https://crrev.com/ad6db10fac0e0a689b91f72a4f34250098584ccc
Cr-Commit-Position: refs/heads/master@{#422328}

https://codereview.chromium.org/2384943002/

tha...@chromium.org

unread,
Oct 3, 2016, 2:11:17 PM10/3/16
to dcheng...@chromium.org, esp...@chromium.org, chromium...@chromium.org, blink-...@chromium.org, dpr...@chromium.org, blink-rev...@chromium.org

https://codereview.chromium.org/2384943002/diff/40001/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
File third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py
(right):

https://codereview.chromium.org/2384943002/diff/40001/third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py#newcode1790
third_party/WebKit/Tools/Scripts/webkitpy/style/checkers/cpp.py:1790:
'Semicolon defining empty statement for this loop. Use { } instead.')
On 2016/10/01 19:48:04, dcheng wrote:
> On 2016/10/01 16:39:46, Nico (mostly away until Oct 3) wrote:
> > I think this is caught by a compiler warning too
>
> At least in a simple test, I wasn't able to get this warning to fire.

It only fires when it's actively buggy:

tthakis@thakis:~/src/chrome/src$ cat test.cc
void f() {
if (1);

if (2)
;

if (3); {
}
}
thakis@thakis:~/src/chrome/src$
third_party/llvm-build/Release+Asserts/bin/clang -c test.cc
test.cc:2:9: warning: if statement has empty body [-Wempty-body]
if (1);
^
test.cc:2:9: note: put the semicolon on a separate line to silence this
warning
test.cc:7:9: warning: if statement has empty body [-Wempty-body]
if (3); {
^
test.cc:7:9: note: put the semicolon on a separate line to silence this
warning
2 warnings generated.



Not sure if check-webkit-style needs to be stricter than that, but
you're right, the compiler does less than what this script does.

https://codereview.chromium.org/2384943002/
Reply all
Reply to author
Forward
0 new messages