1473453 - (clang-bool-pointer-implicit-conversion) Fix and enable clang-tidy's bool-pointer-implicit-conversion warnings

Status: RESOLVED FIXED

(Core :: General, enhancement, P3)

Description

[Chris Peterson [:cpeterson]]

clang-tidy's bool-pointer-implicit-conversion checks for conditions based on implicit conversion from a bool pointer to bool.

  bool* pointer_to_bool;
  if (pointer_to_bool) { // warning: did you mean *pointer_to_bool?
  }

https://clang.llvm.org/extra/clang-tidy/checks/bugprone-bool-pointer-implicit-conversion.html

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: Bug 1473453 - Part 1: a11y: assert `hitregion` property is always null or true.

Fix clang-tidy warning about bool pointers:

[misc-bool-pointer-implicit-conversion] accessible/generic/Accessible.cpp:644:9: warning: dubious check of 'bool *' against 'nullptr', did you mean to dereference it?

The `hitregion` property is always null or points to a true bool, set here:

https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/dom/canvas/CanvasRenderingContext2D.cpp#3936-3937

The `hitregion` property used to be an nsRect pointer:

https://searchfox.org/mozilla-central/diff/c797577640f306df87e8c32313c5b826d1e58a9b/accessible/src/generic/Accessible.cpp#928

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

Comment 2

[Chris Peterson [:cpeterson]]

Attachment: Bug 1473453 - Part 2: QuotaManager: disambiguate null check of bool pointer.

Fix clang-tidy warning about bool pointers:

[misc-bool-pointer-implicit-conversion] dom/quota/ActorsParent.cpp:4168:7: warning: dubious check of 'bool *' against 'nullptr', did you mean to dereference it?

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

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1473453 - Part 3: Enable clang-tidy's misc-bool-pointer-implicit-conversion warning.

And add a clang-tidy test.

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

Comment 4

[Marco Zehe (:MarcoZ)]

Comment on attachment 8989896 [details]
Bug 1473453 - Part 1: a11y: assert `hitregion` property is always null or true.

https://reviewboard.mozilla.org/r/254890/#review261750

Stealing review from Surkov since he's on PTO until end of next week. r=marcoz.

Comment 5

[Andi [:andi]]

thank you for the patch, let's make sure we don't regress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d00ee276ad624a0b06741afa6ad3485a3e288d8

Comment 6

[Andi [:andi]]

Comment on attachment 8989898 [details]
Bug 1473453 - Part 3: Enable clang-tidy's misc-bool-pointer-implicit-conversion warning.

https://reviewboard.mozilla.org/r/254894/#review261762

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to Marco Zehe (:MarcoZ) from comment #4)
> Stealing review from Surkov since he's on PTO until end of next week.
> r=marcoz.

Thanks! :)

Comment 8

[Jan Varga [:janv]]

Comment on attachment 8989897 [details]
Bug 1473453 - Part 2: QuotaManager: disambiguate null check of bool pointer.

https://reviewboard.mozilla.org/r/254892/#review262874

Ok.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f496f2f11ef
Part 1: a11y: assert `hitregion` property is always null or true. r=MarcoZ
https://hg.mozilla.org/integration/autoland/rev/fb7269b3d9b2
Part 2: QuotaManager: disambiguate null check of bool pointer. r=janv
https://hg.mozilla.org/integration/autoland/rev/146466f1d08a
Part 3: Enable clang-tidy's misc-bool-pointer-implicit-conversion warning. r=andi

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7f496f2f11ef
https://hg.mozilla.org/mozilla-central/rev/fb7269b3d9b2
https://hg.mozilla.org/mozilla-central/rev/146466f1d08a

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Bravo, thanks for doing that!