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

Wed Apr 11 03:52:03 PDT 2018

rsmith added inline comments.

================
Comment at: include/clang/AST/ComparisonCategories.h:68
+  /// \brief A map containing the comparison category values built from the
+  /// standard library. The key is a value of ComparisonCategoryValue.
+  llvm::DenseMap<char, DeclRefExpr *> Objects;
----------------
What is ComparisonCategoryValue? Do you mean ComparisonCategoryResult?

================
Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+    const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+    assert(DR &&
----------------
This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have an expression naming the comparison result value, and we shouldn't pretend we do.

================
Comment at: include/clang/AST/ComparisonCategories.h:175-177
+  /// \brief returns true if the comparison category data has already been
+  /// built, and false otherwise.
+  bool hasData() const { return HasData; }
----------------
I don't think this should be exposed. (I think we should only be building the CCK that we need when we request them, rather than building all five, and if we do that, then this function is meaningless.)

================
Comment at: include/clang/AST/ComparisonCategories.h:180
+public:
+  // implementation details which should only be used by the function creating
+  // and setting the data.
----------------
Comments should start with a capital letter.

================
Comment at: include/clang/AST/Expr.h:3097-3106
+  bool isRelationalOp() const {
+    return isRelationalOp(getOpcode()) ||
+           (getOpcode() == BO_Cmp && IsCmpOrdered);
+  }

   static bool isEqualityOp(Opcode Opc) { return Opc == BO_EQ || Opc == BO_NE; }
+  bool isEqualityOp() const {
----------------
These seem wrong to me. Relational operators are `<`, `<=`, `>`, and `>=`. `<=>` is not a relational operator even if it's an ordered comparison. Likewise, `<=>` is not an equality operator even if we don't have an ordered comparison.

If you undo this, you can also drop the `IsCmpOrdered` member entirely.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377-9378
+def err_implied_comparison_category_type_not_found : Error<
+  "%0 type was not found; include <compare> before defining "
+  "or using 'operator<=>'">;
+def err_spaceship_argument_narrowing : Error<
----------------
This doesn't sound quite right. You can define your own `operator<=>` without including `<compare>` so long as you don't try to default it or use one of the standard comparison category kinds. Also, instead of saying "before doing X or Y", say which one the user actually did.

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9384-9385
+def err_spaceship_comparison_of_void_ptr : Error<
+  "three-way comparison with void pointer %select{operand type|operand types}0 "
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<
----------------
Why are you diagnosing this case as an error?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9386-9387
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<
+  "three-way comparison of operands has composite type %0 (%1 and %2)">;
+def err_std_compare_type_missing_member : Error<
----------------
It's unclear to me what this diagnostic is supposed to mean. What actionable feedback is this supposed to be giving a user?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9389
+def err_std_compare_type_missing_member : Error<
+  "%0 missing a member named '%1'">;
+def err_std_compare_type_invalid_member : Error<
----------------
"%0 *is* missing [...]"?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9391
+def err_std_compare_type_invalid_member : Error<
+  "member '%1' of %0 is ill-formed">;
+def note_spaceship_operand_not_cce : Note<
----------------
What do you mean by "ill-formed" here?

I think what you really mean is "we don't understand how to deal with this definition of that member", so maybe we should just fold this and the prior diagnostic into something like "sorry, this standard library implementation of std::whatever is not supported"?

================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9393
+def note_spaceship_operand_not_cce : Note<
+  "argument is not a constant expression">;
 } // end of sema component.
----------------
Is this really useful? I would think almost all the cases where you'd hit the "cannot be narrowed" error, this diagnostic and the explanation of why the operand is not constant would be meaningless noise, because it was never meant to be constant, and the problem is simply that you are trying to three-way compare integers of mixed signedness.

================
Comment at: lib/AST/ExprConstant.cpp:6238-6263
+    bool VisitBinCmp(const BinaryOperator *E) {
+      using CCR = ComparisonCategoryResult;
+      const ComparisonCategoryInfo &CmpInfo =
+          Info.Ctx.CompCategories.getInfoForType(E->getType());
+
+      // Build a new version of the binary operator which returns an integer
+      // representing the ComparisonCategoryResult. Then defer to
----------------
I don't like this approach (inventing an expression that is invalid so that we can call into some other code to compute this result). If this is really materially better than implementing `<=>` directly here, this should be justified by comments here. But I would suspect that factoring out the common code for dealing with the various kinds of comparison from `IntExprEvaluator` and calling it from both here and there would lead to an overall better design.

================
Comment at: lib/AST/ExprConstant.cpp:7107
   IntExprEvaluator(EvalInfo &info, APValue &result)
-    : ExprEvaluatorBaseTy(info), Result(result) {}
+      : ExprEvaluatorBase(info), Result(result) {}

----------------
Historically MSVC has choked on use of the injected-class-name of a base class that is a template specialization. Is this fixed in our supported versions now?

================
Comment at: lib/AST/ExprConstant.cpp:8659
   if (LHSTy->isPointerType() && RHSTy->isPointerType()) {
+    assert(!LHSTy->isMemberPointerType());
     if (E->getOpcode() == BO_Sub || E->isComparisonOp()) {
----------------
This assert adds no value; we checked the type on the previous line. Generally we can assume that people maintaining this code broadly know the language rules.

================
Comment at: lib/AST/ExprConstant.cpp:8959-8961
+
+
+
----------------
Don't need these blank lines :)

================
Comment at: lib/AST/ExprConstant.cpp:10754-10763
+      if (Exp->getOpcode() == BO_Cmp) {
+        // Check that all of the references to the result objects are ICE.
+        const ComparisonCategoryInfo &CmpInfo =
+            Ctx.CompCategories.getInfoForType(Exp->getType());
+        ICEDiag RetDiag(IK_ICE, E->getLocStart());
+        for (const auto &KV : CmpInfo.Objects)
+          RetDiag = Worst(RetDiag, CheckICE(KV.second, Ctx));
----------------
If we want to check this, we should check it when we validate the types in the first place. (Also, I wouldn't put too much effort into this, it can't actually happen. We don't care about the C/C++98 ICE rules in contexts where we produce a `<=>` token.)

================
Comment at: lib/CodeGen/CGExprAgg.cpp:14-16
+#include "CGCXXABI.h"
 #include "CGObjCRuntime.h"
+#include "CodeGenFunction.h"
----------------
This change is incorrect; CodeGenFunction.h is the primary header for this file so should be included first.

================
Comment at: lib/CodeGen/CGExprAgg.cpp:202
   }
+
 };
----------------
No blank line here, please :)

================
Comment at: lib/CodeGen/CGExprAgg.cpp:930-934
+    llvm_unreachable("support for complex types is unimplemented");
+  } else if (ArgTy->isVectorType()) {
+    llvm_unreachable("support for vector types is unimplemented");
+  } else {
+    llvm_unreachable("unexpected argument type when building operator<=>");
----------------
CodeGen has a mechanism for emitting proper "unimplemented" diagnostics; please use that rather than llvm_unreachable.

================
Comment at: lib/CodeGen/CGExprAgg.cpp:955
+  case TEK_Aggregate:
+    // FIXME: Is this ever used?
+    LHS = CGF.EmitAnyExpr(E->getLHS()).getAggregatePointer();
----------------
It's probably used at least for pointers to members in the MS ABI.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8888

+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;
----------------
If you put this on Sema, you'll need to serialize it, and it's no longer just a cache.

I would prefer to treat this information strictly as a cache, and build it in ASTContext. Sema should then just be checking that the information is "valid" when it first makes use of such a comparison category type.

================
Comment at: lib/Sema/SemaExpr.cpp:9895
                                     bool IsRelational) {
+  bool IsSpaceship = Opc == BO_Cmp;
   // Comparisons expect an rvalue, so convert to rvalue before any
----------------
I would prefer this be called `IsThreeWay`; "spaceship" is the name of the operator symbol, but we're talking about the semantic operator here.

https://reviews.llvm.org/D45476