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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/ASTContext.h:1986
+  /// This object needs to be initialized by Sema the first time it checks
+  /// a three-way comparison.
+  ComparisonCategories CompCategories;
----------------
Is this comment accurate?  Because if your test case with a deserialized comparison operator is supposed to work, we certainly don't get that.


================
Comment at: include/clang/AST/ComparisonCategories.h:71
+  /// standard library. The key is a value of ComparisonCategoryResult.
+  mutable llvm::DenseMap<char, VarDecl *> Objects;
+
----------------
We expect this map to have at least two of the seven result types, and probably three or four, right?  It should probably just be an array; it'll both be faster and use less memory.

(The similar map in `ComparisonCategories` is different because we expect it to be empty in most translation units.)


================
Comment at: include/clang/AST/ComparisonCategories.h:206
+  const ASTContext &Ctx;
+  mutable llvm::DenseMap<char, ComparisonCategoryInfo> Data;
+  mutable NamespaceDecl *StdNS = nullptr;
----------------
Please add a comment explaining what the keys are.


================
Comment at: lib/AST/ComparisonCategories.cpp:85
+  return nullptr;
+}
+
----------------
This method is returning a pointer to an entry of a DenseMap.  The resulting pointer is then treated as a stable key in a set on Sema.  That pointer will be dangling if the DenseMap needs to grow beyond its original allocation.

I would suggest perhaps storing a fixed-size array of pointers to ComparisonCategoryInfos that you allocate on-demand.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+    return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };
----------------
EricWF wrote:
> rsmith wrote:
> > Perhaps directly emit the constant value here rather than the address of the global? I think we should consider what IR we want to see coming out of Clang, and I don't think that IR should contain loads from globals to get the small constant integer that is the value of the conversion result.
> > 
> > I think it would be reasonable for us to say that we require the standard library types to contain exactly one non-static data member of integral type, and for us to form a select between the relevant integer values here. We really have no need to support all possible implementations of these types, and we can revisit this if some other standard library implementation ships types that don't follow that pattern. (If we find such a standard library, we could emit multiple selects, or a first-class aggregate select, or whatever generates the best code at -O0.)
> I agree emitting the value would be better, and that most STL implementations should implement the types using only one non-static member.
> However, note that the specification for `partial_ordering` is described in terms of two non-static data members, so it seems possible an STL implementation might implement in that way.
> 
> Would it be appropriate to do this as a smaller follow up patch?
While it would be nice if we could special-case the case of a class with a single integral field all the way through the various uses of it, IRGen is not at all set up to try to take advantage of that.  You will need to store your integral result into the dest slot here anyway.  That makes me suspect that there's just no value in trying to produce a scalar select before doing that store instead of branching to pick one of two stores.

Also, I know there's this whole clever argument for why we can get away with lazily finding this comparison-result type and its static members in translation units that are just deserializing a spaceship operator.  Just to make me feel better, though, please actually check here dynamically that the assumptions we're relying on actually hold and that we've found an appropriate variable for the comparison result and it does have an initializer.  It is fine to generate an atrocious diagnostic if we see a failure, but let's please not crash just because something weird and unexpected happened with module import.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8929
+    return QualType();
+  }
+
----------------
Alright, works for me.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8956
+  // the cache and return the newly cached value.
+  FullyCheckedComparisonCategories.insert(Info);
+  return Info->getType();
----------------
I think you should probably do this insertion above (perhaps instead of the original `count` check) so that you don't dump 100 diagnostics on the user if they've got a malformed stdlib.


================
Comment at: lib/Sema/SemaExpr.cpp:9816
+    RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+    // C++2a [expr.spaceship]p4
----------------
rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > We still need to apply the usual arithmetic conversions after converting enumerations to their underlying types (eg, `<=>` on `enum E : char` converts the operands first to `char` then to `int`). You could remove the `else` here and make this stuff unconditional, but it's probably better to sidestep the extra work and convert directly to the promoted type of the enum's underlying type.
> > Do we still do usual arithmetic conversions if we have two enumerations of the same type?
> Formally, yes: "If both operands have the same enumeration type E, the operator yields the result of converting the operands to the underlying type of E and applying <=> to the converted operands."
> 
> The recursive application of `<=>` to the converted operands will perform the usual arithmetic conversions.
`isEnumeralType` is just checking for an any enum type, but I assume we can't use a spaceship to compare a scoped enum type, right?  Since the ordinary relational operators aren't allowed either.


================
Comment at: lib/Sema/SemaExpr.cpp:9825
+    LHS = S.ImpCastExprToType(LHS.get(), IntType, CK_IntegralCast);
+    RHS = S.ImpCastExprToType(RHS.get(), IntType, CK_IntegralCast);
+    LHSType = RHSType = IntType;
----------------
I believe this will assert if the underlying type of the enum is `bool`.


================
Comment at: lib/Sema/SemaExpr.cpp:9955
+      // and direction polls
+      return buildResultTy(ComparisonCategoryType::StrongEquality);
+
----------------
Should we generate a tautological warning about comparisons on `nullptr_t` that aren't the result of some kind of macro/template expansion?


https://reviews.llvm.org/D45476