[PATCH] D157596: [clang][Interp] Handle mixed floating/integral compound assign operators

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM with a testing nit.



================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2659-2664
+  if (FromT == PT_Float) {
+    // Floating to floating.
+    if (ToT == PT_Float) {
+      const llvm::fltSemantics *ToSem = &Ctx.getFloatSemantics(ToQT);
+      return this->emitCastFP(ToSem, getRoundingMode(E), E);
+    }
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Should we be early returning if we're casting from float->float like we do for int->int?
> Heh, I was thinking the exact same thing when writing this, but 
> 
> a) That means we also have to pass a `QualTyp FromQT`
> b) I don't think we'd ever run into this problem since the AST would have to contain such a cast from/to the same floating type.
Okay, that seems reasonable to me. We can always revisit later if we find it on a hot path for some reason.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:2667
+    // Float to integral.
+    if (isIntegralType(ToT) || ToT == PT_Bool)
+      return this->emitCastFloatingIntegral(ToT, E);
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > It weirds me out that `isIntegralType()` returns false for a boolean type; that is an integral type as well: http://eel.is/c++draft/basic.types#basic.fundamental-11
> I don't think there's a good reason for it to return `false` for bools; before this patch, the function is only used for an assertion in `Neg()`.
Sounds like a good NFC change to make as a follow-up.


================
Comment at: clang/test/AST/Interp/floats.cpp:106
+
+  constexpr float intPlusDouble() {
+   int a = 0;
----------------
tbaeder wrote:
> aaron.ballman wrote:
> > Is it intentional that this is returning a float but then comparing below as an integer?
> Kinda? What we compare against doesn't really matter for the compound assignment in the function, does it?
Heh, that's why I was surprised -- it seems like odd type mismatches that are unrelated to the patch. If that's not what's being tested, might as well match the types up better so the test coverage is more clear.


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

https://reviews.llvm.org/D157596