1370794 - converting integer literal to bool, use bool literal instead

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Sylvestre Ledru [:Sylvestre]]

This replaces some integer literals which will be casted to bool.
https://clang.llvm.org/extra/clang-tidy/checks/modernize-use-bool-literals.html

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1370794 - Use bool instead of integer being casted to bool

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

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8875184 [details]
Bug 1370794 - Use bool instead of integer being casted to bool

https://reviewboard.mozilla.org/r/146586/#review150672

I don't understand a couple of things about this patch:

1. Why this is a good thing?  The clang-tidy page about this cites a bunch of examples which all seem pretty reasonable, but not something like this.  Related: are these really the only changes that this clang-tidy check would make?
2. Why only modify these files, and not numerous others in mfbt or xpcom that have this pattern?
3. `while (0)` is overwhelmingly more common than `while (false)` in the codebase (I think this is still true even without third-party code, but I'm not too sure).  Why are we not switching wholesale and enforcing some sort of lint?

Comment 3

[Sylvestre Ledru [:Sylvestre]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Comment on attachment 8875184 [details]
> Bug 1370794 - Use bool instead of integer being casted to bool
> 
> https://reviewboard.mozilla.org/r/146586/#review150672
> 
> I don't understand a couple of things about this patch:
> 
> 1. Why this is a good thing?  The clang-tidy page about this cites a bunch
> of examples which all seem pretty reasonable, but not something like this. 
Seems that it is good thing as its avoid a cast.
To be honest, I also reported this bug to seek for feedback :)


> Related: are these really the only changes that this clang-tidy check would
> make?
nope, this is only one checker that we have looking at

> 2. Why only modify these files, and not numerous others in mfbt or xpcom
> that have this pattern?
To start small but also because these two files are included a lot.

> 3. `while (0)` is overwhelmingly more common than `while (false)` in the
> codebase (I think this is still true even without third-party code, but I'm
> not too sure).  Why are we not switching wholesale and enforcing some sort
> of lint?
Yeah, this is my end goal!

Comment 4

[(no longer active)]

Is there any reason to not take this patch so that we can fix the rest of the cases incrementally?

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1370794 - Replace some 0 by false in the dom bindings code generation

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

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8875184 [details]
Bug 1370794 - Use bool instead of integer being casted to bool

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/146586/diff/1-2/

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8875184 [details]
Bug 1370794 - Use bool instead of integer being casted to bool

I'm going to give this patch to Ehsan instead; I don't really care for this substitution and can't see the point of it, but he owns C++ style, so I'll let him make the call.

Comment 8

[(no longer active)]

Comment on attachment 8875184 [details]
Bug 1370794 - Use bool instead of integer being casted to bool

https://reviewboard.mozilla.org/r/146586/#review229284

Comment 9

[(no longer active)]

Comment on attachment 8948995 [details]
Bug 1370794 - Replace some 0 by false in the dom bindings code generation

https://reviewboard.mozilla.org/r/218406/#review229286

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fae7bef0ce3
Replace some 0 by false in the dom bindings code generation r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/b96bf2121395
Use bool instead of integer being casted to bool r=Ehsan

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2fae7bef0ce3
https://hg.mozilla.org/mozilla-central/rev/b96bf2121395