[PATCH] D141192: [Clang] Fix warnings on bad shifts.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Sorry for the delay in reviewing, I was out for standards meetings last week and couldn't get to this one. For future patches, can you be sure to upload them with more diff context (`-U9999` is what I usually use).

You should also add a release note for the fix.



================
Comment at: clang/lib/AST/ExprConstant.cpp:2837-2843
+    if (SA != RHS) {
+      if (Info.getLangOpts().CPlusPlus11) {
+        Info.CCEDiag(E, diag::note_constexpr_large_shift)
+            << RHS << E->getType() << LHS.getBitWidth();
+        return false;
+      }
+    }
----------------
These can be combined into one `if` with `&&`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:11762-11763
         << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
-    return;
+
+    // We return BSV_Ok because we already done diag.
+    return BSV_Ok;
----------------
because we already done diag -> because we already emitted the diagnostic


================
Comment at: clang/lib/Sema/SemaExpr.cpp:12023-12025
+  ExpressionEvaluationContext Context = ExprEvalContexts.back().Context;
+  if (Context == ExpressionEvaluationContext::ConstantEvaluated ||
+      Context == ExpressionEvaluationContext::ImmediateFunctionContext) {
----------------
You can use `ExprEvalContexts.back().isConstantEvaluated()` instead.


================
Comment at: clang/test/C/drs/dr0xx.c:429
    */
- _Static_assert(-1 << 1 == -2, "fail"); /* Didn't shift a zero into the "sign bit". */
+ _Static_assert(-1 << 1 == -2, "fail");  /* expected-warning {{shifting a negative signed value is undefined}} */
  _Static_assert(1 << 3 == 1u << 3u, "fail"); /* Shift of a positive signed value does sensible things. */
----------------
The new comment doesn't match the comment above. I'm not certain when WG14 changed the behavior from implementation-defined to undefined, but we should capture that information in the test because the DR has been superseded. I suspect it was from the twos complement changes in C2x though, so that'd be a good place to start looking.


================
Comment at: clang/test/Sema/shift-count-negative.c:7
+enum shiftof {
+    X = (1<<-29) //expected-warning {{shift count is negative}}
+};
----------------
When the shift is a negative *constant* value (as opposed to a signed variable type), should we diagnose this as an error rather than a warning? (Perhaps worth thinking about as a follow-up patch?)


================
Comment at: clang/test/Sema/shift-count-overflow.c:1-2
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wshift-count-overflow %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+
----------------
No need for the `-x c` in these.


================
Comment at: clang/test/Sema/shift-count-overflow.c:5
+enum shiftof {
+    X = (1<<32) // expected-warning {{shift count >= width of type}}   
+};
----------------
This depends on the target architecture, doesn't it? You might want to add some target triples to the RUN line to ensure you've picked targets where this property holds.


================
Comment at: clang/test/Sema/shift-count-overflow.c:7-8
+};
+
+
+
----------------
Spurious newlines?


================
Comment at: clang/test/Sema/shift-negative-value.c:7
+enum shiftof {
+    X = (-1<<29) //expected-warning {{shifting a negative signed value is undefined}}
+};
----------------
This is another case we might want to consider if we can turn into an error (in a follow-up patch).


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

https://reviews.llvm.org/D141192