Closed Bug 1370794 Opened 3 years ago Closed 2 years ago

converting integer literal to bool, use bool literal instead

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Sylvestre, Assigned: Sylvestre)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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 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?
Attachment #8875184 - Flags: review?(nfroyd) → review-
(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!
Is there any reason to not take this patch so that we can fix the rest of the cases incrementally?
See Also: → 1436354
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.
Attachment #8875184 - Flags: review?(nfroyd) → review?(ehsan)
Comment on attachment 8875184 [details]
Bug 1370794 - Use bool instead of integer being casted to bool

https://reviewboard.mozilla.org/r/146586/#review229284
Attachment #8875184 - Flags: review?(ehsan) → review+
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
Attachment #8948995 - Flags: review?(ehsan) → review+
Pushed by sledru@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2fae7bef0ce3
https://hg.mozilla.org/mozilla-central/rev/b96bf2121395
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.