[PATCH] D141414: [clang] add warning on shifting boolean type

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this! It seems that precommit CI found valid failures that should be addressed:

  Clang :: AST/Interp/shifts.cpp
  Clang :: Analysis/svalbuilder-simplify-no-crash.c
  Clang :: CXX/over/over.built/p18.cpp
  Clang :: Misc/warning-flags.c

Please be sure to also add a release note to `clang/docs/ReleaseNotes.rst` for the new warning flag. Also, can you try running this new diagnostic against some larger C and C++ code bases to see if it discovers any false or true positives?



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6606-6610
+def warn_shift_left_on_bool_type : Warning<
+  "left shifting bool relies on implicit cast type width; consider re-write 'bool && (shift_count | desired_type_width - 1)'">,
+  InGroup<DiagGroup<"shift-cast-dependent-overflow">>;
+def warn_shift_right_on_bool_type : Warning<
+  "right shifting bool equals itself if shifting 0 bits or false otherwise; consider re-write 'bool & !shift_count'">;
----------------
Reworded and combined into one diagnostic. I also changed the diagnostic group because "dependent" has specific meaning to C++ folks.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11948-11956
+static void checkBooleanShift(Sema &S, ExprResult &LHS, SourceLocation Loc, BinaryOperatorKind Opc) {
+  if (LHS.get()->IgnoreParenImpCasts()->getType()->isBooleanType()) {
+    if (Opc == BO_Shl) {
+      S.Diag(Loc, diag::warn_shift_left_on_bool_type);
+    } else if (Opc == BO_Shr) {
+      S.Diag(Loc, diag::warn_shift_right_on_bool_type);
+    }
----------------
With the change to the diagnostic wording, I think this function can go away entirely.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11963
   checkArithmeticNull(*this, LHS, RHS, Loc, /*IsCompare=*/false);
+  checkBooleanShift(*this, LHS, Loc, Opc);
 
----------------
One thing I wonder about is whether we can add a fix-it to this diagnostic to rewrite the expression to the alternative form. If that's easy to do, it might be a nice addition to the diagnostic. If it's at all a pain though, I don't insist.


================
Comment at: clang/test/Sema/warn-shift-bool.cpp:1-2
+
+// RUN: %clang_cc1 -fsyntax-only -Wshift-cast-dependent-overflow -fblocks -verify %s
+
----------------



================
Comment at: clang/test/Sema/warn-shift-bool.cpp:6
+  const bool b = 1;
+  return b << x; // expected-warning{{left shifting bool relies on implicit cast type width; consider re-write 'bool && (shift_count | desired_type_width - 1)'}}
+}
----------------
You should also have a test case for `>>`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141414