1258789 - Add WhitespaceAround checkstyle check & fix issues

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This comes up a lot with contributors – let's make review easier.

This is whitespace in expressions like, "1+1" or "1+ 1", which should be "1 + 1".

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

See http://checkstyle.sourceforge.net/config_whitespace.html for all whitespace checks, e.g. we should also include WhitespaceAfter.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add WhitespaceAround for ASSIGN token. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45981/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45981/

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add binary logic operators for WhitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45983/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45983/

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add literal catch & a few others that don't break for WhitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45985/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45985/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add if & return for WhitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45987/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45987/

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add switch for whitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45989/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45989/

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add synchronized & non-failing try to WhitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45991/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45991/

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add while to WhitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45993/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45993/

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add a few more to WhitespaceAround and fix. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45995/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45995/

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add shift operators to WhitespaceAround & fix. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45997/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45997/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add star to WhitespaceAround and fix. r=grisha

Review commit: https://reviewboard.mozilla.org/r/45999/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45999/

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add plus operator to WhitespaceAround and fix. r=grisha

Review commit: https://reviewboard.mozilla.org/r/46001/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46001/

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add passing tokens to WhitespaceAround. r=grisha

Review commit: https://reviewboard.mozilla.org/r/46003/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46003/

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Sorry in advance! :P

This patch series is not yet complete – note that I intend to add all the default markers, which will remove the "tokens" property, which is why I don't care that they're not alphabetized.

Comment 15

[:Grisha Kruglov]

Comment on attachment 8740741 [details]
MozReview Request: Bug 1258789 - Add WhitespaceAround for ASSIGN token. r=grisha

https://reviewboard.mozilla.org/r/45981/#review42511

::: mobile/android/app/checkstyle.xml:56
(Diff revision 1)
>          <module name="OuterTypeFilename"/> <!-- `class Lol` only in Lol.java -->
>          <module name="WhitespaceAfter">
>              <!-- TODO: (bug 1263059) Remove specific tokens to enable CAST check. -->
>              <property name="tokens" value="COMMA, SEMI"/>
>          </module>
> +        <module name="WhitespaceAround">

Fun! I love stuff like this :)

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add rcurly and fix {} issues. r=grisha

Review commit: https://reviewboard.mozilla.org/r/46149/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46149/

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Fix remaining rcurly issues by eliminating double curly bracket. r=grisha

It's an anti-pattern:
  https://blog.jooq.org/2014/12/08/dont-be-clever-the-double-curly-braces-anti-pattern/

Mobile-relevant summary:
  * It creates new types, bloating our APK
  * It can cause memory leaks by keeping a reference to the containing class

Review commit: https://reviewboard.mozilla.org/r/46151/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46151/

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Add div* token for WhitespaceAround. r=grisha

Fix some cast issues while I was at it.

Review commit: https://reviewboard.mozilla.org/r/46153/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46153/

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Remove WhitespaceAround tokens filter and fix lcurly issues. r=grisha

Some issues remain.

Review commit: https://reviewboard.mozilla.org/r/46155/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46155/

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1258789 - Remove allowEmptyLoops I added previously to simplify things. r=grisha

Review commit: https://reviewboard.mozilla.org/r/46157/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46157/

Comment 21

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

That's the last of them! Please review quickly, not because it's important, but because it can cause painful bit-rot!

Comment 22

[:Grisha Kruglov]

Comment on attachment 8741060 [details]
MozReview Request: Bug 1258789 - Remove WhitespaceAround tokens filter and fix lcurly issues. r=grisha

https://reviewboard.mozilla.org/r/46155/#review42637

Comment 23

[:Grisha Kruglov]

Comment on attachment 8741061 [details]
MozReview Request: Bug 1258789 - Remove allowEmptyLoops I added previously to simplify things. r=grisha

https://reviewboard.mozilla.org/r/46157/#review42639

Comment 24

[:Grisha Kruglov]

Comment on attachment 8741059 [details]
MozReview Request: Bug 1258789 - Add div* token for WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/46153/#review42641

Comment 25

[:Grisha Kruglov]

Comment on attachment 8741058 [details]
MozReview Request: Bug 1258789 - Fix remaining rcurly issues by eliminating double curly bracket. r=grisha

https://reviewboard.mozilla.org/r/46151/#review42647

lgtm

Comment 26

[:Grisha Kruglov]

Comment on attachment 8741057 [details]
MozReview Request: Bug 1258789 - Add rcurly and fix {} issues. r=grisha

https://reviewboard.mozilla.org/r/46149/#review42649

i personally like {} more, but oh well.

Comment 27

[:Grisha Kruglov]

Comment on attachment 8740752 [details]
MozReview Request: Bug 1258789 - Add passing tokens to WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/46003/#review42651

Comment 28

[:Grisha Kruglov]

Comment on attachment 8740751 [details]
MozReview Request: Bug 1258789 - Add plus operator to WhitespaceAround and fix. r=grisha

https://reviewboard.mozilla.org/r/46001/#review42655

Comment 29

[:Grisha Kruglov]

Comment on attachment 8740750 [details]
MozReview Request: Bug 1258789 - Add star to WhitespaceAround and fix. r=grisha

https://reviewboard.mozilla.org/r/45999/#review42657

Comment 30

[:Grisha Kruglov]

Comment on attachment 8740749 [details]
MozReview Request: Bug 1258789 - Add shift operators to WhitespaceAround & fix. r=grisha

https://reviewboard.mozilla.org/r/45997/#review42659

Comment 31

[:Grisha Kruglov]

Comment on attachment 8740748 [details]
MozReview Request: Bug 1258789 - Add a few more to WhitespaceAround and fix. r=grisha

https://reviewboard.mozilla.org/r/45995/#review42661

Comment 32

[:Grisha Kruglov]

Comment on attachment 8740747 [details]
MozReview Request: Bug 1258789 - Add while to WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/45993/#review42663

Comment 33

[:Grisha Kruglov]

Comment on attachment 8740746 [details]
MozReview Request: Bug 1258789 - Add synchronized & non-failing try to WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/45991/#review42665

Comment 34

[:Grisha Kruglov]

Comment on attachment 8740745 [details]
MozReview Request: Bug 1258789 - Add switch for whitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/45989/#review42667

Comment 35

[:Grisha Kruglov]

Comment on attachment 8740744 [details]
MozReview Request: Bug 1258789 - Add if & return for WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/45987/#review42669

Comment 36

[:Grisha Kruglov]

Comment on attachment 8740743 [details]
MozReview Request: Bug 1258789 - Add literal catch & a few others that don't break for WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/45985/#review42671

Comment 37

[:Grisha Kruglov]

Comment on attachment 8740742 [details]
MozReview Request: Bug 1258789 - Add binary logic operators for WhitespaceAround. r=grisha

https://reviewboard.mozilla.org/r/45983/#review42675

Comment 38

[:Grisha Kruglov]

Next up, inconsistent indentation!

Comment 39

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to :Grisha Kruglov from comment #26)
> i personally like {} more, but oh well.

I intend to send an email about this one. `{}` is valid in cases where you might want to use it (e.g. empty constructors for util classes) but not for places where you might not want to, e.g. `catch (...) {}` – it's ambiguous to catch, do nothing, and not comment about it.

(In reply to :Grisha Kruglov from comment #38)
> Next up, inconsistent indentation!

Now that's not going to be fun to fix at. all.

Comment 40

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/4aec5484727310c0ddba1d05142095391f570140
Bug 1258789 - Add WhitespaceAround for ASSIGN token. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/2581aa4a58e94998a75878242cbbd6cd4dfd2d0f
Bug 1258789 - Add binary logic operators for WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/e035db2ceba60ba2601130239a9cf591e7381ee5
Bug 1258789 - Add literal catch & a few others that don't break for WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/53f8e479322be4b823b444f562b4dc06c4920b63
Bug 1258789 - Add if & return for WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/30c0b243df2dd2af133a5258c8bce100225285dc
Bug 1258789 - Add switch for whitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/ebe249484944f34aab6da0539be018751c9a5444
Bug 1258789 - Add synchronized & non-failing try to WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/f1a36e0e657e0c0f1fbd0747e0b27dbc3c717a93
Bug 1258789 - Add while to WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/cdfaf8bc0e6c4fdd4138815020c8f371c11a5de8
Bug 1258789 - Add a few more to WhitespaceAround and fix. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/276a287d749033f33188a8b6973df7bfaecfa0d2
Bug 1258789 - Add shift operators to WhitespaceAround & fix. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/914c1cb7842b6d7f4370024c7629a6bb075e6c6f
Bug 1258789 - Add star to WhitespaceAround and fix. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/a7e8cc02fc77d2356a4421ca0224e1c3982517a1
Bug 1258789 - Add plus operator to WhitespaceAround and fix. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/c0364af095efaa7d424931ffd0ddf04570f23a98
Bug 1258789 - Add passing tokens to WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/3281ebb862468d7d7b98ad9fbd439a24c799347a
Bug 1258789 - Add rcurly and fix {} issues. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/7775ee20ad1c1778e040be3a759e2c27073e945c
Bug 1258789 - Fix remaining rcurly issues by eliminating double curly bracket. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/f3d2612a4d276d2180f893773d4ea2a93dddfe59
Bug 1258789 - Add div* token for WhitespaceAround. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/6c7e7542fcd34b5c9c5faa7dd8c85256729961c4
Bug 1258789 - Remove WhitespaceAround tokens filter and fix lcurly issues. r=grisha

https://hg.mozilla.org/integration/fx-team/rev/c07f4c92f8a60d99d1f2fa14704ce33ce347c4f8
Bug 1258789 - Remove allowEmptyLoops I added previously to simplify things. r=grisha

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4aec54847273
https://hg.mozilla.org/mozilla-central/rev/2581aa4a58e9
https://hg.mozilla.org/mozilla-central/rev/e035db2ceba6
https://hg.mozilla.org/mozilla-central/rev/53f8e479322b
https://hg.mozilla.org/mozilla-central/rev/30c0b243df2d
https://hg.mozilla.org/mozilla-central/rev/ebe249484944
https://hg.mozilla.org/mozilla-central/rev/f1a36e0e657e
https://hg.mozilla.org/mozilla-central/rev/cdfaf8bc0e6c
https://hg.mozilla.org/mozilla-central/rev/276a287d7490
https://hg.mozilla.org/mozilla-central/rev/914c1cb7842b
https://hg.mozilla.org/mozilla-central/rev/a7e8cc02fc77
https://hg.mozilla.org/mozilla-central/rev/c0364af095ef
https://hg.mozilla.org/mozilla-central/rev/3281ebb86246
https://hg.mozilla.org/mozilla-central/rev/7775ee20ad1c
https://hg.mozilla.org/mozilla-central/rev/f3d2612a4d27
https://hg.mozilla.org/mozilla-central/rev/6c7e7542fcd3
https://hg.mozilla.org/mozilla-central/rev/c07f4c92f8a6