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