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