[PATCH] D79755: Implement constexpr BinaryOperator for vector types

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2699
+                                    RHSValue.getInt(), Result);
+  assert(LHSValue.getKind() == APValue::Float && "SHhuld be no other options");
+  return handleLogicalOpForVector(LHSValue.getFloat(), Opcode,
----------------
typo: Should


================
Comment at: clang/lib/AST/ExprConstant.cpp:2741
+                                          RHSValue.getInt(), Result);
+  assert(LHSValue.getKind() == APValue::Float && "SHhuld be no other options");
+  return handleCompareOpForVectorHelper(LHSValue.getFloat(), Opcode,
----------------
Typo: Should


================
Comment at: clang/lib/AST/ExprConstant.cpp:2754
+
+  const VectorType *VT = E->getType()->castAs<VectorType>();
+  unsigned NumElements = VT->getNumElements();
----------------
`const auto *`


================
Comment at: clang/lib/AST/ExprConstant.cpp:2778
+    if (EltTy->isIntegerType()) {
+
+      APSInt EltResult{Info.Ctx.getIntWidth(EltTy),
----------------
Might as well remove the spurious whitespace.


================
Comment at: clang/lib/AST/ExprConstant.cpp:2782
+
+      if (BinaryOperator::isLogicalOp(Opcode)) {
+        if (!handleLogicalOpForVector(LHSElt, Opcode, RHSElt, EltResult)) {
----------------
How about:
```
bool Success = true;
if (BinaryOperator::isLogicalOp(Opcode))
  Success = handleLogicalOpForVector(...);
else if (BinaryOperator::isComparisonOp(Opcode))
  Success = handleCompareOpForVector(...);
else
  Success = handleIntIntBinOp(...);

if (!Success) {
  Info.FFDiag(E);
  return false;
}
ResultElements.push_back(...);
```


================
Comment at: clang/lib/AST/ExprConstant.cpp:2799
+      }
+      ResultElements.push_back(APValue(EltResult));
+
----------------
`emplace_back()` instead?


================
Comment at: clang/lib/AST/ExprConstant.cpp:2800
+      ResultElements.push_back(APValue(EltResult));
+
+    } else if (EltTy->isFloatingType()) {
----------------
Spurious newline?


================
Comment at: clang/lib/AST/ExprConstant.cpp:2813
+
+      ResultElements.push_back(APValue(LHSFloat));
+    }
----------------
`emplace_back()` instead?


================
Comment at: clang/lib/AST/ExprConstant.cpp:9688
+    bool VisitBinaryOperator(const BinaryOperator *E);
+    // FIXME: Missing: unary -, unary ~,
     //                 conditional operator (for GNU conditional select),
----------------
Can you re-flow the rest of the comment now that it's been changed?


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

https://reviews.llvm.org/D79755