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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:1002
+  return EmitFinalDestCopy(
+      E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}
----------------
rjmccall wrote:
> Is there something in Sema actually validating that the comparison types is trivially copyable?  Because EmitFinalDestCopy does not work on non-trivial types.
> 
> Also, are we certain that we're allowed to do a copy from an actual global variable here instead of potentially a constexpr evaluation of the variable reference?  And if we are doing a copy, are we registering ODR-uses of all the variables in Sema?
> 
> I don't think you should worry about trying to generate a select between the "actual" comparison result values.  At least not yet.
There is nothing is Sema validating that these comparison types are trivially copyable, and according to the standard they don't need to be.
I assumed we only ended up in `CGExprAgg` if the types were trivially copyable. But I'll look into implementing this for non-trivially copyable comparison types (although we'll probably never actually encounter them).

> Also, are we certain that we're allowed to do a copy from an actual global variable here instead of potentially a constexpr evaluation of the variable reference?

I'm not sure exactly what this means. How would I observe the difference?

>And if we are doing a copy, are we registering ODR-uses of all the variables in Sema?

Probably not. I'll double check that.

> I don't think you should worry about trying to generate a select between the "actual" comparison result values. At least not yet.

I'm not sure exactly what you mean by this.


================
Comment at: lib/Sema/SemaExpr.cpp:9795
+  if (int Count = LHSType->isBooleanType() + RHSType->isBooleanType()) {
+    // TODO: What about bool non-narrowing cases? Like '0' or '1.
+    if (Count != 2) {
----------------
rsmith wrote:
> Missing `'`. Seems like a question to take to Herb and CWG (and maybe EWG), but I suspect the answer will be "this does what we wanted".
> 
> Looks like P0946R0 missed this case of a difference between `<=>` and other operators... oops.
I'll remove the comment for now then.


================
Comment at: lib/Sema/SemaExpr.cpp:9906
+  bool IsThreeWay = Opc == BO_Cmp;
+  auto IsPointerType = [](ExprResult E) {
+    QualType Ty = E.get()->getType().getNonReferenceType();
----------------
rsmith wrote:
> I'd prefer a name that captures that this also recognizes member pointer types.
I'm bad at naming things. Is `IsAnyPointerType` better?


https://reviews.llvm.org/D45476