On Mon, Oct 18, 2021 at 01:23:45PM -0700, Nick Desaulniers wrote: > On Fri, Oct 15, 2021 at 2:44 AM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote: > > > > On Thu, Oct 14, 2021 at 02:57:03PM -0700, Nathan Chancellor wrote: > > > A new warning in clang points out a place in this file where a bitwise > > > OR is being used with boolean expressions: > > > > > > In file included from drivers/staging/wlan-ng/prism2usb.c:2: > > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: warning: use of bitwise '|' with boolean operands [-Wbitwise-instead-of-logical] > > > ((test_and_clear_bit(THROTTLE_RX, &hw->usb_flags) && > > > ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > drivers/staging/wlan-ng/hfa384x_usb.c:3787:7: note: cast one or both operands to int to silence this warning > > > 1 warning generated. > > > > Both sides of this bitwise OR are bool, so | and || are equivalent > > logically. Clang should not warn about it. > > Not when the LHS AND RHS of the the binary operator have side effects, > which is the only condition under which this warning is emitted. RHS > potentially sets a bit, and potentially would not be executed if `|` > was replaced with `||`. Yes. But as in this case, if you silenced the "bitwise-instead-of-logical" warning in the "obvious way" by changing the | to || then it will introduce a bug so it's a risky warning. Ideally everyone would try to understand the code they are changing but that's just not true in real life. What happens is that every single new person compiles the code and sees the warning. There is only one or two people who understand the driver code and a hundred people who compile the code and look at warnings. So there is a slightly over 98% chance that the person looking at the warning doesn't understand the code and they're going to try to fix it. But instead they're going to introduce a bug because they're going to change | to ||. Of course, Nathan and I are a bit experienced so we're careful but other people are less careful and we've seen this over and over where risky warnings just introduce bugs. I saw this a month ago where Smatch complained about a missing error code. It was ancient code so the original author was not arround. A junior dev saw the bug and changed it to return -EINVAL. That Smatch check is very high quality and it did look like it was supposed to be an error path so the patch was back ported to stable. But it turned out that return path was supposed to be success so it broke the boot. I haven't looked in detail. I silenced these warnings in Smatch because my impression at the time was that there was a high false positive rate. That's still my impression, but I haven't looked. It might also be different for different code bases. regards, dan carpenter
