[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: include/clang/AST/ComparisonCategories.h:155-158
+  const ComparisonCategoryInfo &getInfo(ComparisonCategoryKind Kind) const {
+    assert(HasData && "comparison category information not yet built");
+    return Data[static_cast<unsigned>(Kind)];
+  }
----------------
aaron.ballman wrote:
> It seems like this method can be private rather than public.
`getInfo`? No that's used elsewhere.


================
Comment at: include/clang/AST/Expr.h:3020
+  // ordered comparison. Otherwise the comparison is an equality comparison.
+  unsigned IsCmpOrdered : 1;
+
----------------
aaron.ballman wrote:
> It's unfortunate that this means we're using 9 bits rather than the 8 we were previously using, but I don't see a particularly good way to solve that.
It doesn't seem to be an actual problem. That is, it doesn't change the size of `BinaryOperator`. Even sinking the bits into a new Stmt bitfield class has no effect, even though there is room for them there.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:925-929
+  } else if (ArgTy->isIntegralOrEnumerationType() || ArgTy->isPointerType()) {
+    auto Inst =
+        ArgTy->hasSignedIntegerRepresentation() ? InstInfo.SCmp : InstInfo.UCmp;
+    return Builder.CreateICmp(Inst, LHS, RHS, "cmp");
+  } else if (ArgTy->isAnyComplexType()) {
----------------
aaron.ballman wrote:
> You can remove the `else` after a return and just turn these into `if` statements.
I kind of like the  unimplemented cases being chained via `else if` to the implemented ones. It implies that "these are all the cases there are and they're mutually exclusive".


https://reviews.llvm.org/D45476