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

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

2021-10-20 Thread Nico Weber via Phabricator via cfe-commits

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 files. In 16 cases, using && instead of & and 
|| instead of | was possible, but the change was inconsequential [1]. In 6 
cases, & and | were used intentionally to prevent short-circuiting, but it 
wasn't always obvious that that was the case. I usually rewrote the code to 
make it more obvious [2]. In addition to these, there was of course the true 
positive that motivated this warning :)

Apparently we build a (small) subset of LLVM 10 as part of some dependency of 
chrome. The warning fired a few times there, so I turned it off for those files 
(in trunk LLVM it's fixed). I also found a quality-of-implementation bug with 
the warning which I reported as https://bugs.llvm.org/show_bug.cgi?id=52226

1:
https://chromium-review.googlesource.com/c/angle/angle/+/3212889/2/src/libANGLE/renderer/gl/VertexArrayGL.cpp
https://chromium-review.googlesource.com/c/angle/angle/+/3212889/2/src/tests/gl_tests/CopyTexImageTest.cpp
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/content/browser/payments/payment_app_database.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/gin/v8_initializer.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/net/spdy/spdy_http_stream.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/services/network/url_loader.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3212300/2/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc
https://chromium-review.googlesource.com/c/chromium/src/+/3229610/2/chromecast/cast_core/url_rewrite_rules_adapter.cc
https://chromium-review.googlesource.com/c/v8/v8/+/3212891/2/src/objects/feedback-vector.cc
https://chromium-review.googlesource.com/c/v8/v8/+/3231077/2/src/codegen/arm64/assembler-arm64.cc
https://chromium.googlesource.com/external/github.com/google/distributed_point_functions/+/refs/heads/upstream/master%5E%21/#F0
https://dawn-review.googlesource.com/c/dawn/+/66200/2/src/tests/end2end/QueryTests.cpp
https://github.com/KhronosGroup/Vulkan-ValidationLayers/commit/51279399eaecf651d176544d8197f91f48637b2f
https://github.com/netwide-assembler/nasm/pull/17
https://skia-review.googlesource.com/c/skia/+/459456/3/src/core/SkColorFilter.cpp
https://webrtc-review.googlesource.com/c/src/+/234600/2/media/engine/simulcast_encoder_adapter.cc

2:
https://bugs.chromium.org/p/chromium/issues/detail?id=1261591 (still undecided)
https://chromium-review.googlesource.com/c/v8/v8/+/3212891/2/src/compiler/branch-elimination.cc
https://github.com/harfbuzz/harfbuzz/pull/3256
https://skia-review.googlesource.com/c/skia/+/459456/3/modules/skottie/src/animator/Vec2KeyframeAnimator.cpp
https://skia-review.googlesource.com/c/skia/+/459456/3/modules/skottie/src/animator/VectorKeyframeAnimator.cpp
https://skia-review.googlesource.com/c/skia/+/459456/3/src/core/SkPathEffect.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-10-06 Thread Dávid Bolvanský via Phabricator via cfe-commits

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

Thanks, all found cases are not “security/memory” issues but they should use 
logical op to improve code readability (no reason for bitwise op).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-10-06 Thread Sylvestre Ledru via Phabricator via cfe-commits

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
  https://reviews.llvm.org/D108003/new/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-10-03 Thread Dávid Bolvanský via Phabricator via cfe-commits

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).

Repository:
  rG LLVM Github Monorepo

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
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

Index: clang/test/Sema/warn-bitwise-or-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-or-bool.c
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a | b;
+  b = foo() | a;
+  b = (p != 0) | (*p == 42);   // FIXME: also warn for a non-volatile pointer dereference
+  b = foo() | (*q == 42);  // expected-warning {{use of bitwise '|' with boolean operands}}
+   // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  b = foo() | (int)(*q == 42); // OK, no warning expected
+  b = a | foo();
+  b = (int)a | foo(); // OK, no warning expected
+  b = foo() | bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
+  b = foo() | !bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
+  b = foo() | (int)bar(); // OK, no warning expected
+  b = a | baz();
+  b = bar() | FOO;// expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
+  b = foo() | (int)FOO;   // OK, no warning expected
+  b = b | foo();
+  b = bar() | (i > 4);
+  b = (i == 7) | foo();
+#ifdef __cplusplus
+  b = foo() bitor bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+#endif
+
+  if (foo() | bar())  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+;
+
+  sink(a | b);
+  sink(a | foo());
+  sink(foo() | bar());// expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+
+  int n = i + 10;
+  b = (n | (n - 1));
+}
Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation

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

2021-09-30 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

Index: clang/test/Sema/warn-bitwise-or-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-or-bool.c
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a | b;
+  b = foo() | a;
+  b = (p != 0) | (*p == 42);   // FIXME: also warn for a non-volatile pointer dereference
+  b = foo() | (*q == 42);  // expected-warning {{use of bitwise '|' with boolean operands}}
+   // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  b = foo() | (int)(*q == 42); // OK, no warning expected
+  b = a | foo();
+  b = (int)a | foo(); // OK, no warning expected
+  b = foo() | bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
+  b = foo() | !bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
+  b = foo() | (int)bar(); // OK, no warning expected
+  b = a | baz();
+  b = bar() | FOO;// expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-2]]:13-[[@LINE-2]]:14}:"||"
+  b = foo() | (int)FOO;   // OK, no warning expected
+  b = b | foo();
+  b = bar() | (i > 4);
+  b = (i == 7) | foo();
+#ifdef __cplusplus
+  b = foo() bitor bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+#endif
+
+  if (foo() | bar())  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+;
+
+  sink(a | b);
+  sink(a | foo());
+  sink(foo() | bar());// expected-warning {{use of bitwise '|' with boolean operands}}
+  // expected-note@-1 {{cast one or both operands to int to silence this warning}}
+
+  int n = i + 10;
+  b = (n | (n - 1));
+}
Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,63 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+

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

2021-09-30 Thread Nathan Chancellor via Phabricator via cfe-commits

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 that there won't be instances in the future). If 
any happen to show up, I will send fixes for them at that time. In other words, 
LGTM.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-30 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 readability.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-29 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 to int to 
silence warning">;
 

Quuxplusone wrote:
> LGTM, based on the precedent set on line 68. If I were writing all these 
> notes from scratch, I'd say "to silence //this// warning"; but that can be 
> done in a separate PR, if ever.
Yeah, silence this warning is better and also used (well, more often) in this 
file. I will change it to this form.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-29 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/p.patch
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-29 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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">;
 

LGTM, based on the precedent set on line 68. If I were writing all these notes 
from scratch, I'd say "to silence //this// warning"; but that can be done in a 
separate PR, if ever.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-29 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  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
  clang/p.patch
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-26 Thread Dávid Bolvanský via Phabricator via cfe-commits

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-26 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

Index: clang/test/Sema/warn-bitwise-or-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-or-bool.c
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a | b;
+  b = foo() | a;
+  b = (p != 0) | (*p == 42);   // FIXME: also warn for a non-volatile pointer dereference
+  b = foo() | (*q == 42);  // expected-warning {{use of bitwise '|' with boolean operands}}
+  b = foo() | (int)(*q == 42); // OK, no warning expected
+  b = a | foo();
+  b = (int)a | foo(); // OK, no warning expected
+  b = foo() | bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = foo() | !bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = foo() | (int)bar(); // OK, no warning expected
+  b = a | baz();
+  b = bar() | FOO;  // expected-warning {{use of bitwise '|' with boolean operands}}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = foo() | (int)FOO; // OK, no warning expected
+  b = b | foo();
+  b = bar() | (i > 4);
+  b = (i == 7) | foo();
+#ifdef __cplusplus
+  b = foo() bitor bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+#endif
+
+  if (foo() | bar()) // expected-warning {{use of bitwise '|' with boolean operands}}
+;
+
+  sink(a | b);
+  sink(a | foo());
+  sink(foo() | bar()); // expected-warning {{use of bitwise '|' with boolean operands}}
+
+  int n = i + 10;
+  b = (n | (n - 1));
+}
Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,56 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a & b;
+  b = foo() & a;
+  b = (p != 0) & (*p == 42);   // FIXME: also warn for a non-volatile pointer dereference
+  b = foo() & (*q == 42);  // expected-warning {{use of bitwise '&' with boolean operands}}
+  b = foo() & (int)(*q == 42); // OK, no warning expected
+  b = a & foo();
+  b = (int)a & foo(); // OK, no warning expected
+  b = foo() & bar();  // expected-warning {{use of bitwise '&' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & (int)bar(); // OK, no warning expected
+  b = foo() & !bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
+

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

2021-09-26 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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 a `TODO FIXME` comment here? I still think this should be fixed, 
but I'm willing to believe that it's unduly difficult and defer it to later. :)



Comment at: clang/test/Sema/warn-bitwise-and-bool.c:27
+  b = (p != 0) & (*p == 42);
+  b = foo() & (*q == 42); // expected-warning {{use of bitwise '&' with 
boolean operands}}
+  b = a & foo();

For each warning case (or at least for one or two of them), could we also test 
(my suggested) suppression mechanism?  I think it "obviously" just works, but 
it'd be good to test explicitly that it works.
```
b = foo() & (int)(*q == 42);  // OK, no warning expected
```


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-26 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-26 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

Index: clang/test/Sema/warn-bitwise-or-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-or-bool.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a | b;
+  b = foo() | a;
+  b = (p != 0) | (*p == 42);
+  b = foo() | (*q == 42); // expected-warning {{use of bitwise '|' with boolean operands}}
+  b = a | foo();
+  b = foo() | bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = foo() | !bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = a | baz();
+  b = bar() | FOO; // expected-warning {{use of bitwise '|' with boolean operands}}
+   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = b | foo();
+  b = bar() | (i > 4);
+  b = (i == 7) | foo();
+#ifdef __cplusplus
+  b = foo() bitor bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+#endif
+
+  if (foo() | bar()) // expected-warning {{use of bitwise '|' with boolean operands}}
+;
+
+  sink(a | b);
+  sink(a | foo());
+  sink(foo() | bar()); // expected-warning {{use of bitwise '|' with boolean operands}}
+
+  int n = i + 10;
+  b = (n | (n - 1));
+}
Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbitwise-instead-of-logical -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a & b;
+  b = foo() & a;
+  b = (p != 0) & (*p == 42);
+  b = foo() & (*q == 42); // expected-warning {{use of bitwise '&' with boolean operands}}
+  b = a & foo();
+  b = foo() & bar();  // expected-warning {{use of bitwise '&' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & !bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = a & baz();
+  b = bar() & FOO; // expected-warning {{use of bitwise '&' with boolean operands}}
+   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = b & foo();
+  b = bar() & (i > 4);
+  b = (i == 7) & foo();
+#ifdef __cplusplus
+  b = foo() bitand bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
+#endif
+
+  if (foo() & bar()) // expected-warning {{use of bitwise '&' with boolean operands}}
+;
+
+  sink(a & b);
+  sink(a

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

2021-09-09 Thread Ryan Beltran via Phabricator via cfe-commits

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 
`|`.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/perfetto/src/perfetto_cmd/perfetto_cmd.cc;l=492?q=%22(is_attach()%20%7C%20is_detach()%20%7C%7C%20query_service_%20%7C%7C%20has_config_options))%22

https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v4.4/drivers/gpu/drm/i915/i915_drv.h?q=%22i915_reset_in_progress(error)%20%7C%20i915_terminally_wedged(error)%22

https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform2/power_manager/powerd/policy/charge_controller.cc?q=%22ApplyPeakShiftChange(policy)%20%26%20ApplyBootOnAcChange(policy)%20%26%22

https://source.chromium.org/chromium/chromium/src/+/main:third_party/angle/src/libANGLE/renderer/gl/VertexArrayGL.cpp?q=%22const%20bool%20enabled%20%3D%20mState.getVertexAttribute(attribIndex).enabled%20%26%22

https://source.chromium.org/chromium/chromium/src/+/main:third_party/harfbuzz-ng/src/src/hb-ot-layout-gpos-table.hh?q=%22if%20(valueFormats%5B0%5D.apply_value%20(c,%20this,%20%26record-%3Evalues%5B0%5D,%20buffer-%3Ecur_pos())%20%7C%22

https://source.chromium.org/chromium/chromium/src/+/main:third_party/harfbuzz-ng/src/src/hb-ot-layout-gpos-table.hh?q=%22if%20(valueFormat1.apply_value%20(c,%20this,%20v,%20buffer-%3Ecur_pos())%20%7C%22


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-06 Thread Hans Wennborg via Phabricator via cfe-commits

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:
> > Quuxplusone wrote:
> > > I suggest that the name and wording of this diagnostic should match 
> > > `warn_logical_instead_of_bitwise`, currently `"use of logical '%0' with 
> > > constant operand"`. So:
> > > ```
> > > def warn_bitwise_instead_of_logical : Warning<
> > >   "use of bitwise '%0' with boolean operand">,
> > > ```
> > > This neatly sidesteps the problem of what to call the `&` operator: I was 
> > > not thrilled with the phrase `bitwise and of`, but have no problem with 
> > > `use of bitwise '&'`.
> > I see the point but then I will not be able to provide -Wbool-operation-and 
> > flag to enable/disable this warning.
> > 
> > For example I know that Google prefers a new flag for every new warning so 
> > they dont have to disable eg. -Wbool-operation, but just this new warning 
> > while there are working on fixes for new warnings.
> > 
> > @hans what do you think?
> > 
> > 
> I don't understand your comment. My guess is that you didn't notice that 
> `-Wbitwise-instead-of-logical` would still be a different flag from 
> `-Wlogical-instead-of-bitwise`. I'm not suggesting putting them under the 
> //same// flag; just making the newly added flag a little more consistent and 
> (heh) logical, based on what's already there.
> @hans what do you think?

I think we're all in agreement here. Putting this behind a separate 
-Wbitwise-instead-of-logical flag sounds good to me.



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 you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"

xbolva00 wrote:
> Quuxplusone wrote:
> > So the warning triggers only when the RHS has side-effects? I'd like tests 
> > for
> > ```
> > foo() & a;  // should not trigger, by that logic, because && wouldn't 
> > short-circuit anything?
> > (p != nullptr) & (*p == 42);  // should certainly trigger: deref is a side 
> > effect, and of course obviously this is a bug
> > ```
> Yes, but oh.
> 
> This is weird, HasSideEffects is false for this case. Works fine for volatile 
> int.
> 
> @rsmith?
I don't think a non-volatile pointer deref actually has any side effect. I 
think what we'd want to check here is if the expression could be reordered with 
the LHS. But I don't know how we'd check that.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-04 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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 
https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ : if we see the 
programmer writing `somebool | foo`, we //know// they've messed up — they might 
have meant either `int(somebool) | foo` or `somebool || foo`, we're not sure 
//which//, but we know they've messed up //somehow//. So it makes sense to tell 
them about it. The codepaths for `&&/&` and `||/|` are going to be shared, 
right? So I see no reason to special-case-//not//-warn on `|`.
I do agree that it seems reasonable for people to screw up `&` more frequently 
than `|`. It's very common in C and C++ to type just a single `&` (e.g. for 
address-of); it's very uncommon to ever type a single `|`, so the muscle memory 
won't be there to typo it. Also, `|` is way off on the side of the keyboard 
where the typist is probably paying a little more attention; `&` is right in 
the hot path where our fingers are likely moving quickly. But from the 
compiler's POV, we might be //surprised// to see the rare `|`-for-`||` typo 
("whoa! I hardly ever see one of those!") but that's no reason to keep it a 
secret from the user.




Comment at: clang/test/Sema/warn-bitwise-or-bool.c:38
+#ifdef __cplusplus
+  b = foo() bitor bar(); // expected-warning {{use of bitwise '|' with boolean 
operands}}
+#endif

Arguably, `foo() bitor bar()` is a sufficiently high "degree of ornamentation" 
to unambiguously signal the programmer's intent here. What are the chances that 
the programmer wrote `bitor` instead of `or` by accident?  But it's not worth 
adding any code just to special-case-//not//-warn here.
(The reverse — writing `or` when you meant `bitor` — strikes me as a more 
likely error.)
D107294 is related.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-09-04 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-27 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-24 Thread Ryan Beltran via Phabricator via cfe-commits

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 pattern?

I don't actually have a case off-hand that I've spotted from the real world, 
but this example snippet seems plausible enough:

  if (queue.empty() || queue.pop() == STOP_TOKEN) {
break;
  }

Consider that in this scenario, we only consider queue.pop() if the queue is 
not empty due to short circuiting.

  if (queue.empty() | queue.pop() == STOP_TOKEN) {
break;
  }

While this scenario calls pop() regardless, likely causing a runtime error if 
the queue is empty.

I'll run this warning against ChromeOS and see if I spot any interesting 
results.

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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Dávid Bolvanský via Phabricator via cfe-commits

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/

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread 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 
, 
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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Ryan Beltran via Phabricator via cfe-commits

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 in LLVM where we want to run a bunch of 
>> transformations and get if any of them changed anything.
>>
>> https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/audio/echo_canceller3_config.cc;drc=cbdbb8c1666fd08a094422905e73391706a05b03;l=111
>
> The remaining places where this fired looked like places where `&&` would be 
> better than `&`, except for one where the code was treating bools as one bit 
> integers and doing various bitwise operations on them
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/distributed_point_functions/src/dpf/distributed_point_function.cc;drc=87b84b3834343e141ec94e3321f4d1c7be8a7a9d;l=230



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 in LLVM where we want to run a bunch of 
>> transformations and get if any of them changed anything.
>>
>> https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/audio/echo_canceller3_config.cc;drc=cbdbb8c1666fd08a094422905e73391706a05b03;l=111
>
> The remaining places where this fired looked like places where `&&` would be 
> better than `&`, except for one where the code was treating bools as one bit 
> integers and doing various bitwise operations on them
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/distributed_point_functions/src/dpf/distributed_point_function.cc;drc=87b84b3834343e141ec94e3321f4d1c7be8a7a9d;l=230

New revision should be more conversative to avoid warning on cases you 
mentioned.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Dávid Bolvanský via Phabricator via cfe-commits

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)
- Added support for bitwise | + tests.


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

https://reviews.llvm.org/D108003

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-bitwise-and-bool.c
  clang/test/Sema/warn-bitwise-or-bool.c

Index: clang/test/Sema/warn-bitwise-or-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-or-bool.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a | b;
+  b = foo() | a;
+  b = (p != 0) | (*p == 42);
+  b = foo() | (*q == 42); // expected-warning {{use of bitwise '|' with boolean operands}}
+  b = a | foo();
+  b = foo() | bar();  // expected-warning {{use of bitwise '|' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = foo() | !bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = a | baz();
+  b = bar() | FOO; // expected-warning {{use of bitwise '|' with boolean operands}}
+   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"||"
+  b = b | foo();
+  b = bar() | (i > 4);
+  b = (i == 7) | foo();
+#ifdef __cplusplus
+  b = foo() bitor bar(); // expected-warning {{use of bitwise '|' with boolean operands}}
+#endif
+
+  if (foo() | bar()) // expected-warning {{use of bitwise '|' with boolean operands}}
+;
+
+  sink(a | b);
+  sink(a | foo());
+  sink(foo() | bar()); // expected-warning {{use of bitwise '|' with boolean operands}}
+
+  int n = i + 10;
+  b = (n | (n - 1));
+}
Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,50 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a & b;
+  b = foo() & a;
+  b = (p != 0) & (*p == 42);
+  b = foo() & (*q == 42); // expected-warning {{use of bitwise '&' with boolean operands}}
+  b = a & foo();
+  b = foo() & bar();  // expected-warning {{use of bitwise '&' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & !bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = a & baz();
+  b = bar() & FOO; // expected-warning {{use of bitwise '&' with boolean operands}}
+   // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = b & foo();
+  b = bar() & (i > 4);
+  b = (i == 7) & foo();
+#ifdef __cplusplus
+  b = foo() bitand bar(); // expected-warning {{use of bitwise '&' with boolean operands}}
+#endif
+
+  if (foo() & bar()) // expected-warning {{use of bitwise '&' with boolean operands}}
+;
+
+  sink(a & b);
+  sink(a & foo());
+  sink(foo() & bar()); // expected-warning {{use of bitwise '&' with boolean operands}}
+
+  int n = i + 10;
+  b = (n & (n - 1));
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp

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

2021-08-19 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 for "|" vs "||". It seems like both operators 
> have roughly the same potential for confusion. Could we add support for 
> bitwise vs logical or in this?

Yeah, I mentioned in the first comment as part of “open questions”. Basically 
at first we need to find out some reasonable “heuristics” when to warn and then 
support for | should be added.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Ryan Beltran via Phabricator via cfe-commits

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 confusion. Could we add support for bitwise 
vs logical or in this?


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-19 Thread Arthur Eubanks via Phabricator via cfe-commits

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 anything.
>
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/audio/echo_canceller3_config.cc;drc=cbdbb8c1666fd08a094422905e73391706a05b03;l=111

The remaining places where this fired looked like places where `&&` would be 
better than `&`, except for one where the code was treating bools as one bit 
integers and doing various bitwise operations on them
https://source.chromium.org/chromium/chromium/src/+/main:third_party/distributed_point_functions/src/dpf/distributed_point_function.cc;drc=87b84b3834343e141ec94e3321f4d1c7be8a7a9d;l=230


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-16 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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 && something`. So any 
compiler suggestion or fixit that blindly "doubles down" on the 
non-short-circuiting behavior is a bad idea. 
https://quuxplusone.github.io/blog/2020/09/02/wparentheses/ is relevant.

One proper way to silence the warning //without// short-circuiting would be `a 
= int(a) & something` — this indicates that you really do want integer 
bitwise-ANDing, not boolean. Alternatively, and probably best, just declare `a` 
as an integer variable to begin with. Of course the compiler cannot recommend 
this if `something` isn't strictly zero or one. But in that case you might 
already have a bug:

  int error_returning_func() { return 2; }
  int main() {
  bool t = true;
  t = t && error_returning_func();
  assert(t);  // still true
  t = t & error_returning_func();
  assert(!t);  // now it's false
  }

So typo'ing `&` for `&&` doesn't just turn off short-circuiting — it can also 
mess up the runtime value stored in the bool.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-16 Thread Arthur Eubanks via Phabricator via cfe-commits

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 in LLVM where we want to run a bunch of 
>> transformations and get if any of them changed anything.
>>
>> https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/audio/echo_canceller3_config.cc;drc=cbdbb8c1666fd08a094422905e73391706a05b03;l=111
>
> Maybe “res &= foo ();” would be better? “Changed |= foo();” is very typical 
> for LLVM.

Can we either emit that as a suggestion if the code looks like `a = a & foo()`? 
Or simply not emit a warning?


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-bitwise-and-bool.c


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13101,6 +13101,16 @@
   << OrigE->getSourceRange() << T->isBooleanType()
   << FixItHint::CreateReplacement(UO->getBeginLoc(), "!");
 
+  if (const auto *BO = dyn_cast(SourceExpr))
+if (BO->getOpcode() == BO_And &&
+BO->getLHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->HasSideEffects(S.Context))
+  S.Diag(BO->getBeginLoc(), diag::warn_bitwise_instead_of_logical)
+  << "&"
+  << "&&" << OrigE->getSourceRange()
+  << FixItHint::CreateReplacement(BO->getOperatorLoc(), "&&");
+
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
   if (auto *CO = dyn_cast(SourceExpr)) {
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7421,6 +7421,9 @@
   "member %0 declared here">;
 def note_member_first_declared_here : Note<
   "member %0 first declared here">;
+def warn_bitwise_instead_of_logical : Warning<
+  "use of bitwise '%0' with boolean operands; did you mean logical '%1'?">,
+  InGroup, DefaultIgnore;
 def warn_bitwise_negation_bool : Warning<
   "bitwise negation of a boolean expression%select{;| always evaluates to 
'true';}0 "
   "did you mean logical negation?">,
Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c -fsyntax-only -Wbool-operation 
-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -Wbool-operation 
-fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a & b;
+  b = foo() & a;
+  b = (p != 0) & (*p == 42);
+  b = (q != 0) & (*q == 42); // expected-warning {{use of bitwise '&' with 
boolean operands; did you mean logical '&&'?}}
+  b = a & foo();  // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = foo() & bar();  // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & !bar(); // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = a & baz();
+  b = a & FOO;// expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = !b & foo(); // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"&&"
+  b = bar() & (i > 4);
+  b = (i == 7) & foo(); // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+#ifdef __cplusplus
+  b = a bitand foo(); // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+#endif
+
+  if (foo() & bar()) // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+;
+
+  sink(a & b);
+  sink(a & foo()); // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+  sink(foo() & bar()); // expected-warning {{se of bitwise '&' with boolean 
operands; did you mean logical '&&'?}}
+
+  int n = i + 10;
+  b = (n & (n-1));
+}
\ No newline at end of file


Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp

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

2021-08-15 Thread Dávid Bolvanský via Phabricator via cfe-commits

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-14 Thread Nathan Chancellor via Phabricator via cfe-commits

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.

https://lore.kernel.org/r/20210814234248.1755703-1-nat...@kernel.org/
https://lore.kernel.org/r/20210814235625.1780033-1-nat...@kernel.org/
https://lore.kernel.org/r/20210815004154.1781834-1-nat...@kernel.org/

Is there a reason `-Wbool-operation` is not in `-Wall`? It is with GCC 
(although now, their `-Wbool-operation` is equivalent to just 
`Wbool-operation-negation`. I know that is tangential to this patch but if we 
want to take advantage of this new warning, we will have to explicitly enable 
`-Wbool-operation` in the kernel's Makefile.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 anything.
>
> https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/audio/echo_canceller3_config.cc;drc=cbdbb8c1666fd08a094422905e73391706a05b03;l=111

Maybe “res &= foo ();” would be better? “Changed |= foo();” is very typical for 
LLVM.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Nathan Chancellor via Phabricator via cfe-commits

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 write `&&` and only wrote `&` by accident. The third 
> one might even be a bug bug, since it's doing essentially `(bounds-check 
> offset_1) & (pointer-math-on offset_1)`.

Agreed, I do plan to send patches to fix these up. I will test the warning 
against a wider range of code later to help evaluate how noisy it is and look 
for false positives.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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:
> > I suggest that the name and wording of this diagnostic should match 
> > `warn_logical_instead_of_bitwise`, currently `"use of logical '%0' with 
> > constant operand"`. So:
> > ```
> > def warn_bitwise_instead_of_logical : Warning<
> >   "use of bitwise '%0' with boolean operand">,
> > ```
> > This neatly sidesteps the problem of what to call the `&` operator: I was 
> > not thrilled with the phrase `bitwise and of`, but have no problem with 
> > `use of bitwise '&'`.
> I see the point but then I will not be able to provide -Wbool-operation-and 
> flag to enable/disable this warning.
> 
> For example I know that Google prefers a new flag for every new warning so 
> they dont have to disable eg. -Wbool-operation, but just this new warning 
> while there are working on fixes for new warnings.
> 
> @hans what do you think?
> 
> 
I don't understand your comment. My guess is that you didn't notice that 
`-Wbitwise-instead-of-logical` would still be a different flag from 
`-Wlogical-instead-of-bitwise`. I'm not suggesting putting them under the 
//same// flag; just making the newly added flag a little more consistent and 
(heh) logical, based on what's already there.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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 doing essentially `(bounds-check offset_1) & 
(pointer-math-on offset_1)`.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/intel/iwlwifi/mvm/scan.c#n830
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8192u/r8192U_core.c#n4268
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/zstd/compress.c#n1294

Data point: I've run this patch over my employer's medium-sized mostly-modern 
codebase and found no (true or false) positives at all.


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Arthur Eubanks via Phabricator via cfe-commits

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.

https://source.chromium.org/chromium/chromium/src/+/main:third_party/webrtc/api/audio/echo_canceller3_config.cc;drc=cbdbb8c1666fd08a094422905e73391706a05b03;l=111


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 suggestion?


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-13 Thread Nathan Chancellor via Phabricator via cfe-commits

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]
drivers/staging/rtl8192u/r8192U_core.c:4268:20: warning: bitwise and of
boolean expressions; did you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1043:7: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1075:30: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1294:7: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1353:30: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1932:7: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1956:22: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:1977:23: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]
lib/zstd/compress.c:2024:29: warning: bitwise and of boolean expressions; did
you mean logical and? [-Wbool-operation-and]

Which correspond to:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/wireless/intel/iwlwifi/mvm/scan.c#n830

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/rtl8192u/r8192U_core.c#n4268

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/zstd/compress.c#n1294

I can do `allmodconfig` builds later today.

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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-12 Thread Dávid Bolvanský via Phabricator via cfe-commits

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:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-bitwise-and-bool.c

Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, volatile int *q, int i) {
+  b = a & b;
+  b = foo() & a;
+  b = (p != 0) & (*p == 42);
+  b = (q != 0) & (*q == 42); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  b = a & foo();  // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = foo() & bar();  // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & !bar(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = a & baz();
+  b = a & FOO;// expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = !b & foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"&&"
+  b = bar() & (i > 4);
+  b = (i == 7) & foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+#ifdef __cplusplus
+  b = a bitand foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+#endif
+
+  if (foo() & bar()) // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+;
+
+  sink(a & b);
+  sink(a & foo()); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  sink(foo() & bar()); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+
+  int n = i + 10;
+  b = (n & (n-1));
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13073,6 +13073,15 @@
   << OrigE->getSourceRange() << T->isBooleanType()
   << FixItHint::CreateReplacement(UO->getBeginLoc(), "!");
 
+  if (const auto *BO = dyn_cast(SourceExpr))
+if (BO->getOpcode() == BO_And &&
+BO->getLHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->HasSideEffects(S.Context))
+  S.Diag(BO->getBeginLoc(), diag::warn_bitwise_and_bool)
+  << OrigE->getSourceRange()
+  << FixItHint::CreateReplacement(BO->getOperatorLoc(), "&&");
+
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
   if (auto *CO = dyn_cast(SourceExpr)) {
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7424,7 +7424,10 @@
 def warn_bitwise_negation_bool : Warning<
   "bitwise negation of a boolean expression%select{;| always evaluates to 'true';}0 "
   "did you mean logical negation?">,
-  InGroup>;
+  InGroup;
+def warn_bitwise_and_bool : Warning<
+  "bitwise and of boolean expressions; did you mean logical and?">,
+  InGroup;
 def err_decrement_bool : Error<"cannot decrement expression of type bool">;
 def warn_increment_bool : Warning<
   "incrementing expression of type bool is deprecated and "
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++

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

2021-08-12 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"

Quuxplusone wrote:
> So the warning triggers only when the RHS has side-effects? I'd like tests for
> ```
> foo() & a;  // should not trigger, by that logic, because && wouldn't 
> short-circuit anything?
> (p != nullptr) & (*p == 42);  // should certainly trigger: deref is a side 
> effect, and of course obviously this is a bug
> ```
Yes, but oh.

This is weird, HasSideEffects is false for this case. Works fine for volatile 
int.

@rsmith?


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-12 Thread Dávid Bolvanský via Phabricator via cfe-commits

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 name and wording of this diagnostic should match 
> `warn_logical_instead_of_bitwise`, currently `"use of logical '%0' with 
> constant operand"`. So:
> ```
> def warn_bitwise_instead_of_logical : Warning<
>   "use of bitwise '%0' with boolean operand">,
> ```
> This neatly sidesteps the problem of what to call the `&` operator: I was not 
> thrilled with the phrase `bitwise and of`, but have no problem with `use of 
> bitwise '&'`.
I see the point but then I will not be able to provide -Wbool-operation-and 
flag to enable/disable this warning.

For example I know that Google prefers a new flag for every new warning so they 
dont have to disable eg. -Wbool-operation, but just this new warning while 
there are working on fixes for new warnings.

@hans what do you think?




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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-12 Thread Marshall Clow via Phabricator via cfe-commits

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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-12 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/warn-bitwise-and-bool.c

Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,51 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+void sink(boolean);
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int *p, int i) {
+  b = a & b;
+  b = foo() & a;
+  b = (p != 0) & (*p == 42);
+  b = a & foo();  // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = foo() & bar();  // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & !bar(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = a & baz();
+  b = a & FOO;// expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = !b & foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"&&"
+  b = bar() & (i > 4);
+  b = (i == 7) & foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+#ifdef __cplusplus
+  b = a bitand foo(); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+#endif
+
+  if (foo() & bar()) // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+;
+
+  sink(a & b);
+  sink(a & foo()); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+  sink(foo() & bar()); // expected-warning {{bitwise and of boolean expressions; did you mean logical and?}}
+
+  int n = i + 10;
+  b = (n & (n-1));
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13073,6 +13073,15 @@
   << OrigE->getSourceRange() << T->isBooleanType()
   << FixItHint::CreateReplacement(UO->getBeginLoc(), "!");
 
+  if (const auto *BO = dyn_cast(SourceExpr))
+if (BO->getOpcode() == BO_And &&
+BO->getLHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->HasSideEffects(S.Context))
+  S.Diag(BO->getBeginLoc(), diag::warn_bitwise_and_bool)
+  << OrigE->getSourceRange()
+  << FixItHint::CreateReplacement(BO->getOperatorLoc(), "&&");
+
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
   if (auto *CO = dyn_cast(SourceExpr)) {
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7424,7 +7424,10 @@
 def warn_bitwise_negation_bool : Warning<
   "bitwise negation of a boolean expression%select{;| always evaluates to 'true';}0 "
   "did you mean logical negation?">,
-  InGroup>;
+  InGroup;
+def warn_bitwise_and_bool : Warning<
+  "bitwise and of boolean expressions; did you mean logical and?">,
+  InGroup;
 def err_decrement_bool : Error<"cannot decrement expression of type bool">;
 def warn_increment_bool : Warning<
   "incrementing expression of type bool is deprecated and "
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -64,6 +64,10 @@
 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;

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

2021-08-12 Thread Arthur O'Dwyer via Phabricator via cfe-commits

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?">,
+  InGroup;

I suggest that the name and wording of this diagnostic should match 
`warn_logical_instead_of_bitwise`, currently `"use of logical '%0' with 
constant operand"`. So:
```
def warn_bitwise_instead_of_logical : Warning<
  "use of bitwise '%0' with boolean operand">,
```
This neatly sidesteps the problem of what to call the `&` operator: I was not 
thrilled with the phrase `bitwise and of`, but have no problem with `use of 
bitwise '&'`.



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 you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"

So the warning triggers only when the RHS has side-effects? I'd like tests for
```
foo() & a;  // should not trigger, by that logic, because && wouldn't 
short-circuit anything?
(p != nullptr) & (*p == 42);  // should certainly trigger: deref is a side 
effect, and of course obviously this is a bug
```



Comment at: clang/test/Sema/warn-bitwise-and-bool.c:31
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  i = !b & foo(); // expected-warning {{bitwise and of boolean expressions; 
did you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"&&"

Why is this assigning to `i` instead of `b`?
Actually, the assignment shouldn't matter to the diagnostic at all, right? 
Perhaps each test case should be written as, like,
```
void sink(bool);

   sink(a & b);
   sink(a & foo());
   sink(foo() & bar());
```
etc.



Comment at: clang/test/Sema/warn-bitwise-and-bool.c:34
+  b = bar() & (i > 4);
+  b = (i == 7) & foo(); // expected-warning {{bitwise and of boolean 
expressions; did you mean logical and?}}
+#ifdef __cplusplus

I'd also like a negative test for @mclow.lists' example of `int n = ...; b = (n 
& (n-1));` (where the //result// is being used as a boolean, but the bitwise op 
is correct and the logical op would be 100% wrong).



Comment at: clang/test/Sema/warn-bitwise-and-bool.c:36
+#ifdef __cplusplus
+  b = a and foo();
+#endif

I assume you meant `bitand` and expect a warning?


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

https://reviews.llvm.org/D108003

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2021-08-12 Thread Dávid Bolvanský via Phabricator via cfe-commits

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
  clang/test/Sema/warn-bitwise-and-bool.c


Index: clang/test/Sema/warn-bitwise-and-bool.c
===
--- /dev/null
+++ clang/test/Sema/warn-bitwise-and-bool.c
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | 
FileCheck %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wbool-operation %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 
| FileCheck %s
+
+#ifdef __cplusplus
+typedef bool boolean;
+#else
+typedef _Bool boolean;
+#endif
+
+boolean foo(void);
+boolean bar(void);
+boolean baz(void) __attribute__((const));
+
+#define FOO foo()
+
+void test(boolean a, boolean b, int i) {
+  b = a & b;
+  b = a & foo();  // expected-warning {{bitwise and of boolean 
expressions; did you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  b = foo() & bar();  // expected-warning {{bitwise and of boolean 
expressions; did you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = foo() & !bar(); // expected-warning {{bitwise and of boolean 
expressions; did you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:13-[[@LINE-1]]:14}:"&&"
+  b = a & baz();
+  b = a & FOO;// expected-warning {{bitwise and of boolean expressions; 
did you mean logical and?}}
+  // CHECK: fix-it:"{{.*}}":{[[@LINE-1]]:9-[[@LINE-1]]:10}:"&&"
+  i = !b & foo(); // expected-warning {{bitwise and of boolean expressions; 
did you mean logical and?}}
+  // CHECK: 
fix-it:"{{.*}}":{[[@LINE-1]]:10-[[@LINE-1]]:11}:"&&"
+  b = bar() & (i > 4);
+  b = (i == 7) & foo(); // expected-warning {{bitwise and of boolean 
expressions; did you mean logical and?}}
+#ifdef __cplusplus
+  b = a and foo();
+#endif
+}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -13073,6 +13073,15 @@
   << OrigE->getSourceRange() << T->isBooleanType()
   << FixItHint::CreateReplacement(UO->getBeginLoc(), "!");
 
+  if (const auto *BO = dyn_cast(SourceExpr))
+if (BO->getOpcode() == BO_And &&
+BO->getLHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->isKnownToHaveBooleanValue() &&
+BO->getRHS()->HasSideEffects(S.Context))
+  S.Diag(BO->getBeginLoc(), diag::warn_bitwise_and_bool)
+  << OrigE->getSourceRange()
+  << FixItHint::CreateReplacement(BO->getOperatorLoc(), "&&");
+
   // For conditional operators, we analyze the arguments as if they
   // were being fed directly into the output.
   if (auto *CO = dyn_cast(SourceExpr)) {
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7424,7 +7424,10 @@
 def warn_bitwise_negation_bool : Warning<
   "bitwise negation of a boolean expression%select{;| always evaluates to 
'true';}0 "
   "did you mean logical negation?">,
-  InGroup>;
+  InGroup;
+def warn_bitwise_and_bool : Warning<
+  "bitwise and of boolean expressions; did you mean logical and?">,
+  InGroup;
 def err_decrement_bool : Error<"cannot decrement expression of type bool">;
 def warn_increment_bool : Warning<
   "incrementing expression of type bool is deprecated and "
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -64,6 +64,10 @@
 def SignConversion : DiagGroup<"sign-conversion">;
 def PointerBoolConversion : DiagGroup<"pointer-bool-conversion">;
 def UndefinedBoolConversion : DiagGroup<"undefined-bool-conversion">;
+def BoolOperationNegation : DiagGroup<"bool-operation-negation">;
+def BoolOperationAnd : DiagGroup<"bool-operation-and">;
+def BoolOperation : DiagGroup<"bool-operation", [BoolOperationNegation,
+ BoolOperationAnd]>;
 def BoolConversion : DiagGroup<"bool-conversion", [PointerBoolConversion,
UndefinedBoolConversion]>;
 def IntConversion : DiagGroup<"int-conversion">;


Index: clang/test/Sema/warn-bitwise-and-bool.c

50 matches

Mail list logo