Bug 30021 – check-webkit-style does not detect missing spaces around &=, |=, -=, and +=

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Carol Szabo]

While these binary operators require spaces around them (per the webkit style guidelines) check-webkit-style currently misses these errors, despite detecting similar errors involving ==, !=, <= and >=.
Fix to follow shortly

[Carol Szabo]

Created attachment 40536 [details]
Proposed patch

This patch addresses lack of white space detection for a lot of binary operators.

[Eric Seidel (no email)]

Comment on attachment 40536 [details]
Proposed patch

I need to see more unit testing before I can be sure this is correct.

[Carol Szabo]

Created attachment 40548 [details]
Proposed patch

Added some unit tests, but I think the best test is the fact that over the entire existing WebKit codebase, there is no change in the output of the tool introduced by my changes.
This means that at least my change is very unlikely to hide common errors that were previously detected and very unlikely to introduce a lot of annoying false errors.
My recorded unit tests as well as running the tool on my own patches proves that it finds a few things that were  not found before.
By the way, I noticed that check-webkit-style is intentionally coded to allow constructs such as a = 3<<4; This exception from the normal "binary operator rule" is not mentioned in the coding styles webpage or I have missed it (http://webkit.org/coding/coding-style.html). Also, another exception is if ((a=b) != c) construction where a comment in the check-webkit-style file says that there must NOT be spaces around = inside if. This exception is not mentioned in the guidelines either.

[Darin Adler]

(In reply to comment #3)
> I noticed that check-webkit-style is intentionally coded to allow
> constructs such as a = 3<<4;

That sounds wrong. WebKit coding style would require spaces there.

> Also, another exception is if
> ((a=b) != c) construction where a comment in the check-webkit-style file says
> that there must NOT be spaces around = inside if.

That's wrong. My guess is that it came from someone else's coding style guidelines. Maybe Google internal guidelines?

[Carol Szabo]

(In reply to comment #4)
> (In reply to comment #3)
> > I noticed that check-webkit-style is intentionally coded to allow
> > constructs such as a = 3<<4;
> 
> That sounds wrong. WebKit coding style would require spaces there.
> 
> > Also, another exception is if
> > ((a=b) != c) construction where a comment in the check-webkit-style file says
> > that there must NOT be spaces around = inside if.
> 
> That's wrong. My guess is that it came from someone else's coding style
> guidelines. Maybe Google internal guidelines?

Thanks for the clarification. If I'll edit the check-webkit-style file again I shall fix those, but till then I'd like my current fixes landed so that I do not have to manually add them to my new workspaces.

[David Levin]

Comment on attachment 40548 [details]
Proposed patch

Looks good.  Thanks for making it better!

btw, about the allowed items that shouldn't be allowed, check-webkit-style is a recent addition to WebKit and thus may be more liberal than desired (due to its heritage from checking Google style).

[WebKit Commit Bot]

Comment on attachment 40548 [details]
Proposed patch

Clearing flags on attachment: 40548

Committed r49078: <http://trac.webkit.org/changeset/49078>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.