[PATCH] D108003: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side effects

[Nathan Chancellor via Phabricator via cfe-commits]

nathanchance added a comment.

I am still running a series of builds against the Linux kernel but I already see one instance of this warning where the suggestion would change the meaning of the code in an incorrect way:

  drivers/input/touchscreen.c:81:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
          data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/input/touchscreen.c:81:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
          data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-x",
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/input/touchscreen.c:94:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
          data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/input/touchscreen.c:94:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
          data_present = touchscreen_get_prop_u32(dev, "touchscreen-min-y",
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  drivers/input/touchscreen.c:108:17: warning: use of bitwise '|' with boolean operands [-Wbool-operation]
          data_present = touchscreen_get_prop_u32(dev,
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  5 warnings generated.

Which corresponds to this file: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/input/touchscreen.c?h=v5.14-rc6

If the calls to `touchscreen_get_prop_u32` short circuit, we could use `maximum` or `fuzz` uninitialized. There might be a cleaner way to rewrite that block to avoid the warning but based on the other instances of this warning I see, I am not sure `|` vs. `||` is worth warning about (but I am happy to hear examples of how it could be a bug). Most people realize `&&` short circuits (as `if (a && foo(a->...))` is relatively common) but most probably are not thinking about `||` short circuiting (and it would be more of an optimization than correctness issue as far as I understand it).

Additionally, I have not caught any instances of `&` being used instead of `&&`, including the ones I notated previously; those were caught because only the right side had side effects. As was pointed out here and on the mailing list <https://lore.kernel.org/r/defb9e5133234835950c21511d776fb9@AcuMS.aculab.com/>, the `lib/zstd/` warning is probably a bug, as the short circuit should happen if `offset_1` is zero, otherwise there is unnecessary work done.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003