aaron.ballman 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}}
----------------
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.
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
