[PATCH] D136532: [clang][Interp] Implement left and right shifts

[Timm Bäder via Phabricator via cfe-commits]

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