[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 3 inline comments as done.
mibintc added a comment.

In D76384#1929774 <https://reviews.llvm.org/D76384#1929774>, @mibintc wrote:

> There's a bug in this patch that i haven't solved yet. I think it's just 1 bug. The bug is that the AST statement visitor is going to the "fallback" instead of the right destination when walking CompoundAssignmentOperator.  This patch collapses CompoundAssignmentOperator class into BinaryOperator class.  In a spot check, i think all the failing tests are because the Visitor walk isn't working correctly for CompoundAssign (for example += ). These are the test fails reported by check-clang in my sandbox,
>  Failing Tests (15):
>
>   Clang :: Analysis/loopexit-cfg-output.cpp
>   Clang :: CXX/drs/dr2xx.cpp
>   Clang :: CXX/expr/expr.const/p2-0x.cpp
>   Clang :: CXX/expr/expr.prim/expr.prim.lambda/expr.prim.lambda.capture/p17.cpp
>   Clang :: CodeGen/ffp-contract-fast-option.cpp
>   Clang :: CodeGenCXX/const-init-cxx1y.cpp
>   Clang :: CodeGenCXX/const-init-cxx2a.cpp
>   Clang :: CodeGenCXX/non-const-init-cxx2a.cpp
>   Clang :: Sema/warn-unreachable.c
>   Clang :: Sema/warn-unsequenced.c
>   Clang :: SemaCXX/constant-expression-cxx1y.cpp
>   Clang :: SemaCXX/constant-expression-cxx2a.cpp
>   Clang :: SemaCXX/decomposed-condition.cpp
>   Clang :: SemaCXX/integer-overflow.cpp
>   Clang :: SemaCXX/warn-unsequenced.cpp


The new revision corrects all these fails. i have a couple questions about test cases, i've inline those in the test case modification. Also need to do clang-format and also some changes to CXXCallOperator to remove redundant accessor



================
Comment at: clang/include/clang/AST/ExprCXX.h:168
   // operations on floating point types.
   void setFPFeatures(FPOptions F) {
+    FPFeatures = F;
----------------
i can get rid of these 2 accessor functions, we get them from CallExpr


================
Comment at: clang/test/Analysis/loopexit-cfg-output.cpp:211
 // CHECK:       [B3]
-// CHECK-NEXT:   1: j
-// CHECK-NEXT:   2: 2
-// CHECK-NEXT:   3: [B3.1] += [B3.2]
+// CHECK-NEXT:   1: 2
+// CHECK-NEXT:   2: j
----------------
With these changes, the blocks print out in a different order but the semantic meaning appears to be the same. Is this difference acceptable? 


================
Comment at: clang/test/Sema/warn-unreachable.c:86
   case 8:
-    i
-      +=        // expected-warning {{will never be executed}}
+    i           // expected-warning {{will never be executed}}
+      +=
----------------
The srcpos for compound assignment is now the same as srcpos for ordinary binary operator, so i changed the test case to move the diagnostic,  ok?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76384