[PATCH] D134859: [clang][Interp] Implement basic support for floating point values

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

tbaeder added inline comments.


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:152
+  case CK_FloatingToIntegral: {
+    llvm::RoundingMode RM = getRoundingMode(CE);
+    Optional<PrimType> ToT = classify(CE->getType());
----------------
sepavloff wrote:
> According to C standard (6.3.1.4p1):
> ```
>     When a finite value of real floating type is converted to an integer type other than _Bool, the fractional part is discarded (i.e., the value is truncated toward zero).
> ```
> So the conversion should not depend on rounding mode. The same applies to the conversion to boolean (6.3.1.2p1):
> ```
>     When any scalar value is converted to _Bool, the result is 0 if the value compares equal to 0; otherwise, the result is 1.
> ```
> 
So this is a difference between C and C++?


================
Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:617
   case BO_SubAssign:
     if (!this->emitSub(*LT, E))
       return false;
----------------
sepavloff wrote:
> This and some subsequent operations also should depend on rounding mode?
Yes, I did `add` as an example, the others will have to follow.


================
Comment at: clang/lib/AST/Interp/Floating.h:109
+    Floating RHS = B;
+    if (!A.compatibleWith(B))
+      RHS = B.toSemantics(A, RM);
----------------
sepavloff wrote:
> Do we really need this check? In AST operands of addition always have the same type.
In https://godbolt.org/z/s4n75jc4h, the LHS of the `CompoundAssignOperator` is float while the RHS is double


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

https://reviews.llvm.org/D134859