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

thakis added a comment. Thanks for this warning! I just finished turning it on in Chromium. Here's a short report on the things it fired on in case it's interesting to anyone. (I'm not suggesting any changes to the warning, I think it works very well as-is). The warning fired in 21 different

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

xbolva00 added a comment. In D108003#3044853 , @sylvestre.ledru wrote: > It found a few issues on Firefox: > https://bugzilla.mozilla.org/show_bug.cgi?id=1734285 > > I think it should be added it in the release notes: > https://reviews.llvm.org/D111215

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

sylvestre.ledru added a comment. It found a few issues on Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1734285 I think it should be added it in the release notes: https://reviews.llvm.org/D111215 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGf62d18ff140f: [Clang] Extend -Wbool-operation to warn about bitwise and of bools with side… (authored by xbolva00).

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

xbolva00 updated this revision to Diff 376279. xbolva00 added a comment. Updated warning-wall.c test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticGroups.td

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

nathanchance added a comment. As far as I am aware, this iteration of the patch has no instances in the Linux kernel now. The instances I found with the first iteration of the patch only had a right hand side with side effects, not both sides, so this warning is effectively a no-op now (not

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

xbolva00 marked an inline comment as done. xbolva00 added a comment. @rpbeltran @nathanchance @aaron.ballman Are you OK with the current state of this patch? Well, it is clear that some code in linux kernel/Chromium/etc needs to adjusted, but I think in many cases it would (atleast) improve

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

xbolva00 marked 3 inline comments as done. xbolva00 added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:69 def note_cast_to_void : Note<"cast expression to void to silence warning">; +def note_cast_operand_to_int : Note<"cast one or both operands

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

xbolva00 updated this revision to Diff 375969. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticGroups.td clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaChecking.cpp

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

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:69 def note_cast_to_void : Note<"cast expression to void to silence warning">; +def note_cast_operand_to_int : Note<"cast one or both operands to int to silence warning">;

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

xbolva00 updated this revision to Diff 375960. xbolva00 added a comment. Herald added subscribers: sstefan1, s.egerton, simoncook, aheejin, dschuff. Herald added a reviewer: jdoerfert. Emit note to explain a user how to silence this warning. CHANGES SINCE LAST ACTION

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

xbolva00 added a comment. Maybe we should emit note like “cast one or both operands to int to silence this warning” (any idea for better note text?)? I think it could be useful. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003

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

xbolva00 updated this revision to Diff 375118. xbolva00 edited the summary of this revision. xbolva00 added a comment. Addressed comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticGroups.td

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

Quuxplusone added a comment. LGTM % comments, FWIW. Comment at: clang/test/Sema/warn-bitwise-and-bool.c:26 + b = foo() & a; + b = (p != 0) & (*p == 42); + b = foo() & (*q == 42); // expected-warning {{use of bitwise '&' with boolean operands}} Perhaps add

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

xbolva00 added a comment. Looks OK now? or something more to be done? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

xbolva00 updated this revision to Diff 375115. xbolva00 added a comment. Introduced -Wbitwise-instead-of-logical flag. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticGroups.td

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

rpbeltran added a comment. Sorry for the late reply on this. I ran the warning against ChromeOS and actually found very few false positives and a couple interesting findings to file bugs for. I actually saw about an equal number of cases caught for `&` and `|`.

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

hans added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7428-7430 +def warn_bitwise_and_bool : Warning< + "bitwise and of boolean expressions; did you mean logical and?">, + InGroup; Quuxplusone wrote: > xbolva00 wrote: > >

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

Quuxplusone added a comment. In D108003#2983609 , @xbolva00 wrote: > I think I will start with AND only as this is more error prone pattern. FWIW, I still see no reason //not// to warn on `|`-for-`||`. Again per

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

xbolva00 added a comment. Ping I think I will start with AND only as this is more error prone pattern. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 ___ cfe-commits mailing list

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

xbolva00 added a comment. >> I'll run this warning against ChromeOS and see if I spot any interesting >> results. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 ___ cfe-commits mailing

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

rpbeltran added a comment. In D108003#2956332 , @xbolva00 wrote: >>> and it would be more of an optimization than correctness issue as far as I >>> understand > > Yeah, this is right, indeed. > > Maybe @rpbeltran has some idea or motivating cases for OR

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

xbolva00 added a comment. >> and it would be more of an optimization than correctness issue as far as I >> understand Yeah, this is right, indeed. Maybe @rpbeltran has some idea or motivating cases for OR pattern? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/

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

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

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

rpbeltran added a comment. Thanks! And sorry for missing that point in your first comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

xbolva00 added a comment. @rpbeltran please check new revision, I added support for bitwise OR. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 ___ cfe-commits mailing list

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

xbolva00 added a comment. In D108003#2955009 , @aeubanks wrote: > In D108003#2944178 , @aeubanks > wrote: > >> I ran this over Chrome and ran into a use case that looks legitimate. It >> seems like the pattern

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

xbolva00 updated this revision to Diff 367611. xbolva00 edited the summary of this revision. xbolva00 added a comment. - Only warn when both sides have potentional side effects (conversative, but covers motivating case, reduces useless noise - which may hide real bug - caused by this warning) -

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

xbolva00 added a comment. In D108003#2955058 , @rpbeltran wrote: > This patch seems like a great contribution! Really glad to see this being > added. I did have a question though on why this only appears to catch "&" vs > "&&" instead of doing the same

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

rpbeltran added a comment. This patch seems like a great contribution! Really glad to see this being added. I did have a question though on why this only appears to catch "&" vs "&&" instead of doing the same for "|" vs "||". It seems like both operators have roughly the same potential for

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

aeubanks added a comment. In D108003#2944178 , @aeubanks wrote: > I ran this over Chrome and ran into a use case that looks legitimate. It > seems like the pattern in LLVM where we want to run a bunch of > transformations and get if any of them changed

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

Quuxplusone added a comment. > Can we either emit that [`a &= foo()`] as a suggestion if the code looks like > `a = a & foo()`? Or simply not emit a warning? The problem is that if `a` is boolean, then `a = a & something` is 99% for sure a typo — 99% of the time, the programmer meant `a = a &&

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

aeubanks added a comment. In D108003#2944714 , @xbolva00 wrote: > In D108003#2944178 , @aeubanks > wrote: > >> I ran this over Chrome and ran into a use case that looks legitimate. It >> seems like the pattern

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

xbolva00 updated this revision to Diff 366490. xbolva00 added a comment. Rebased. Changed the warning text. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td

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

xbolva00 added a comment. -Wbool-operation is basically enabled even without -Wall, but yeah, gcc only warns with -Wall specified. I will change it as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003

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

nathanchance added a comment. I have sent the following patches for the Linux kernel. These were the only instances of the warning that I found across the code base in all of the configurations that we usually test so thank you for the heads up so I could send fixes before this lands.

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

xbolva00 added a comment. In D108003#2944178 , @aeubanks wrote: > I ran this over Chrome and ran into a use case that looks legitimate. It > seems like the pattern in LLVM where we want to run a bunch of > transformations and get if any of them changed

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

nathanchance added a comment. In D108003#2944413 , @Quuxplusone wrote: > FWIW, all three of @nathanchance's detected lines look like good true > positives to me: even if they're not //bugs//, they all look like places the > programmer //meant// to

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

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7428-7430 +def warn_bitwise_and_bool : Warning< + "bitwise and of boolean expressions; did you mean logical and?">, + InGroup; xbolva00 wrote: > Quuxplusone wrote:

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

Quuxplusone added a comment. FWIW, all three of @nathanchance's detected lines look like good true positives to me: even if they're not //bugs//, they all look like places the programmer //meant// to write `&&` and only wrote `&` by accident. The third one might even be a bug bug, since it's

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

aeubanks added a comment. I ran this over Chrome and ran into a use case that looks legitimate. It seems like the pattern in LLVM where we want to run a bunch of transformations and get if any of them changed anything.

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

xbolva00 added a comment. Thanks! Well, for those cases, && instead of & looks like a better choice. WDYT? Or is it a very noisy? Restrict more so both sides must have side effects? Maybe we could suggest to use cast - (int)boolRHS - as a way how to silence this warning.. Or any better

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

nathanchance added a comment. With several Linux kernel `defconfig` builds, I see the following instances of the warning: drivers/net/wireless/intel/iwlwifi/mvm/scan.c:835:3: warning: bitwise and of boolean expressions; did you mean logical and? [-Wbool-operation-and]

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

xbolva00 updated this revision to Diff 366143. xbolva00 marked an inline comment as not done. xbolva00 added a comment. New test with volatile int. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files:

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

xbolva00 marked an inline comment as not done. xbolva00 added inline comments. Comment at: clang/test/Sema/warn-bitwise-and-bool.c:21-22 +void test(boolean a, boolean b, int i) { + b = a & b; + b = a & foo(); // expected-warning {{bitwise and of boolean expressions; did

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

xbolva00 added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7428-7430 +def warn_bitwise_and_bool : Warning< + "bitwise and of boolean expressions; did you mean logical and?">, + InGroup; Quuxplusone wrote: > I suggest that the

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

mclow.lists added a comment. I suggest you take all the techniques at http://graphics.stanford.edu/~seander/bithacks.html and make sure they don't cause a warning. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003

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

xbolva00 updated this revision to Diff 366139. xbolva00 added a comment. Added new tests. Thanks for recommendations. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticGroups.td

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

Quuxplusone added subscribers: mclow.lists, Quuxplusone. Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7428-7430 +def warn_bitwise_and_bool : Warning< + "bitwise and of boolean expressions; did you mean logical and?">, +

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

xbolva00 updated this revision to Diff 366130. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108003/new/ https://reviews.llvm.org/D108003 Files: clang/include/clang/Basic/DiagnosticGroups.td clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaChecking.cpp