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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 24 inline comments as done.
EricWF added inline comments.


================
Comment at: lib/AST/ExprConstant.cpp:3784
+static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD,
+                            APValue *Dest = nullptr) {
   // We don't need to evaluate the initializer for a static local.
----------------
rsmith wrote:
> This `Dest` parameter seems to be unused, is it left behind from a previous direction?
Woops. It was, thanks!


================
Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+  return EmitFinalDestCopy(
+      E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}
----------------
rsmith wrote:
> Hm, I wonder whether it's worthwhile to try to generate a select between the comparison result values rather than their addresses. (Maybe not, since they could in general be an arbitrary aggregate type, and a select on a first-class aggregate value is unlikely to produce anything useful at -O0).
I was thinking something similar, but like you said, the STL implementation could provide an arbitrary aggregate type. 
However, I think that's a candidate for a follow up patch, so I'll add a `TODO` about it and leave it for now.



================
Comment at: lib/Sema/SemaExpr.cpp:9727-9728
+
+static bool checkNarrowingConversion(Sema &S, QualType ToType, Expr *E,
+                                     QualType FromType, SourceLocation Loc) {
+  // Check for a narrowing implicit conversion.
----------------
rsmith wrote:
> This should have a name that has something to do with three-way comparisons (that is, assuming that duplicating this is the best way to customize the diagnostic behavior).
I'm not sure this is the cleanest way to do it, but it seems better than trying to integrate it more directly with the `CheckConvertedConstantExpression` machinery. The semantics for `operator<=>` seems just different enough.

That being said, I'm very open to suggestions. You're the expert and resident compiler wizard.


================
Comment at: lib/Sema/SemaExpr.cpp:9807-9810
+    if (!S.Context.hasSameUnqualifiedType(LHSType, RHSType)) {
+      S.InvalidOperands(Loc, LHS, RHS);
+      return QualType();
+    }
----------------
rsmith wrote:
> Please implement the "straight to CWG" resolutions from http://wiki.edg.com/bin/view/Wg21jacksonville2018/P0946R0-Jax18 directly here. Specifically, in this case, we should allow three-way comparisons between unscoped enumerations and integral types (subject to the narrowing check), but not between two unrelated enumeration types, and not between a scoped enumeration and an integral type.
I was thinking that if there wasn't already an issue for this behavior, there should be. Thanks for pointing it out.


================
Comment at: lib/Sema/SemaExpr.cpp:11942
     ConvertHalfVec = true;
     ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, true);
+    assert(ResultTy.isNull() || ResultTy->getAsCXXRecordDecl());
----------------
rsmith wrote:
> Ah, here it is, `true` is incorrectly being passed for `IsRelational` here. Maybe replace that `bool` with an `enum` (or remove it entirely and have the callee recompute it from `Opc`)?
Ack. Removing the parameter and re-computing it from `Opc`.


https://reviews.llvm.org/D45476