tbaeder added inline comments.
================
Comment at: clang/test/AST/Interp/shifts.cpp:57
+ //c >>= 999999; // expected-warning {{shift count >= width of type}}
+ //c <<= CHAR_BIT; // expected-warning {{shift count >= width of type}}
+ //c >>= CHAR_BIT; // expected-warning {{shift count >= width of type}}
----------------
aaron.ballman wrote:
> shafik wrote:
> > aaron.ballman wrote:
> > > shafik wrote:
> > > > aaron.ballman wrote:
> > > > > tbaeder wrote:
> > > > > > shafik wrote:
> > > > > > > This is not correct, the operands go through integral promotions
> > > > > > > first and the result is the promoted type of the left operand see
> > > > > > > [dcl.shift p1](https://eel.is/c++draft/expr.shift#1).
> > > > > > >
> > > > > > > Also see godbolt: https://godbolt.org/z/7qzKjojMb
> > > > > > Hmm, this is copy-pasted from `test/SemaCXX/shift.cpp`.
> > > > > FWIW, I agree with Shafik -- you can also see the casts in the AST:
> > > > > https://godbolt.org/z/97dqr1TEs
> > > > This weird, you can see this is indeed valid in a constant expression:
> > > > https://godbolt.org/z/W33janMxv
> > > >
> > > > but we do obtain that diagnostic and I think I see what it is saying,
> > > > that the result type is 8bit but the shift is larger than that type
> > > > even though the operation is being done on the promoted type. I feel
> > > > like the diagnostic is not great but could be improved to make it
> > > > better.
> > > >
> > > > wdyt @aaron.ballman
> > > > This weird, you can see this is indeed valid in a constant expression:
> > > > https://godbolt.org/z/W33janMxv
> > >
> > > Yes, because of the integral promotion of `c` to `int`.
> > >
> > > > but we do obtain that diagnostic and I think I see what it is saying,
> > > > that the result type is 8bit but the shift is larger than that type
> > > > even though the operation is being done on the promoted type. I feel
> > > > like the diagnostic is not great but could be improved to make it
> > > > better.
> > >
> > > I think it's telling the programmer that something strange is happening
> > > here. Yes, it's well-defined because of integral promotions, but it still
> > > looks like suspect code. We could say "width of the unpromoted type" but
> > > that might make the diagnostic more confusing for non-language lawyer
> > > folks.
> > > > but we do obtain that diagnostic and I think I see what it is saying,
> > > > that the result type is 8bit but the shift is larger than that type
> > > > even though the operation is being done on the promoted type. I feel
> > > > like the diagnostic is not great but could be improved to make it
> > > > better.
> > >
> > > I think it's telling the programmer that something strange is happening
> > > here. Yes, it's well-defined because of integral promotions, but it still
> > > looks like suspect code. We could say "width of the unpromoted type" but
> > > that might make the diagnostic more confusing for non-language lawyer
> > > folks.
> >
> > I am bothered that the same diagnostic message represents two cases 1) a
> > case which is undefined behavior 2) a case which may be surprising but well
> > defined.
> >
> > I am bothered that the same diagnostic message represents two cases 1) a
> > case which is undefined behavior 2) a case which may be surprising but well
> > defined.
>
> That's reasonable, but short of users complaining about significant false
> positives or asking us to split the situations into two so they can control
> the difference, I don't think there's anything to be fixed here. The goal of
> the diagnostic isn't to point out only the UB cases, it's to point out
> "you're doing something deeply weird, are you sure you mean to do that?"
> which happens to cover the UB cases too.
In any case, I've enabled those test cases and the output with the new
interpreter is the same one we get with the current interpreter.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D136532/new/
https://reviews.llvm.org/D136532
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
