[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant
Richard Smith - zygoloid via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Sat Apr 14 07:55:51 PDT 2018
rsmith added a comment.
Thanks! This is looking pretty close.
================
Comment at: include/clang/AST/ComparisonCategories.h:78
+public:
+ /// \brief Wether Sema has fully checked the type and result values for this
+ /// comparison category types before.
----------------
Typo "Wether".
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377
+def err_implied_comparison_category_type_not_found : Error<
+ "cannot deduce return type of operator<=> because type %0 was not found; "
+ "include <compare>">;
----------------
Nit: you have inconsistent quoting of 'operator<=>' between this diagnostic and the next.
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9387
+ "standard library implementation of comparison category %0 is not supported; "
+ "failed to build reference to member '%1'">;
} // end of sema component.
----------------
This seems a bit too implementation-detaily -- how about something vaguer like "[...] not supported; member '%1' does not have expected form"?
(I suppose it doesn't matter too much; only standard library implementers are likely to ever see this diagnostic anyway.)
================
Comment at: lib/AST/ComparisonCategories.cpp:51
+ if (!StdNS) {
+ StdNS = NamespaceDecl::Create(
+ const_cast<ASTContext &>(Ctx), Ctx.getTranslationUnitDecl(),
----------------
We shouldn't be creating a 'namespace std' here. If there is no existing such namespace, our lookups should just fail.
================
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.
----------------
This `Dest` parameter seems to be unused, is it left behind from a previous direction?
================
Comment at: lib/AST/ExprConstant.cpp:8824
+ return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, []() -> bool {
+ llvm_unreachable("operator<=> should have been evaluated to a result");
+ });
----------------
I'm not convinced this is a legitimate use of `llvm_unreachable` -- it seems to me that there could be all sorts of types for which Sema can form a `<=>` expression but that we don't handle here, such as vector types. Maybe issue an `FFDiag` here, or just call the base class version of this function, which I think should do it for you?
================
Comment at: lib/CodeGen/CGExprAgg.cpp:947-954
+ return CGF.ErrorUnsupported(
+ E, "aggregate binary expression with complex arguments");
+ if (ArgTy->isVectorType())
+ return CGF.ErrorUnsupported(
+ E, "aggregate binary expression with vector arguments");
+ if (!ArgTy->isIntegralOrEnumerationType() && !ArgTy->isRealFloatingType() &&
+ !ArgTy->isPointerType() && !ArgTy->isMemberPointerType())
----------------
Instead of "aggregate binary expression" in all of these errors, how about "three-way comparison"?
================
Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+ return EmitFinalDestCopy(
+ E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}
----------------
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).
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8903-8916
+ // Build the initial category information
+ RecordDecl *CCDecl = nullptr;
+ // Lookup the record for the category type
+ if (auto Std = getStdNamespace()) {
+ LookupResult Result(*this, &PP.getIdentifierTable().get(Name),
+ SourceLocation(), Sema::LookupTagName);
+ if (LookupQualifiedName(Result, Std))
----------------
I don't think this makes sense: `CompCategories::lookupInfo` already did the lookup we wanted here; checking some other declaration just invites the two lookups being inconsistent in some way. I think you should just check whether `CachedInfo` is null here, and if so, produce the "type not found" error.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8936-8948
+ // Build each of the require values and store them in Info.
+ for (CCVT CCV : Values) {
+ StringRef ValueName = ComparisonCategories::getResultString(CCV);
+ QualType Ty(CCDecl->getTypeForDecl(), 0);
+ DeclContext *LookupCtx = computeDeclContext(Ty);
+ LookupResult Found(*this, &PP.getIdentifierTable().get(ValueName), Loc,
+ Sema::LookupOrdinaryName);
----------------
Likewise here, you should query the `CachedInfo` object for these values and check them, rather than looking them up again in a different way.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8966
+ assert(CachedInfo->Kind == Kind);
+ CachedInfo->IsFullyChecked = true;
+ return QualType(CachedInfo->CCDecl->getTypeForDecl(), 0);
----------------
I would somewhat prefer that we keep (say) a `DenseSet` of checked types on `Sema` rather than having `Sema` effectively own this flag that's held by `ASTContext`.
================
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.
----------------
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).
================
Comment at: lib/Sema/SemaExpr.cpp:9761-9776
+ if (E->isValueDependent())
+ return false;
+
+ // Check the expression is a constant expression.
+ SmallVector<PartialDiagnosticAt, 8> Notes;
+ Expr::EvalResult Eval;
+ Eval.Diag = &Notes;
----------------
Delete all of this? Once we get this far, we know it's a narrowing conversion, so should just be diagnosing it.
================
Comment at: lib/Sema/SemaExpr.cpp:9794
+
+ // C++2a [expr.spaceship]p3
+ if (int Count = (LHSType->isBooleanType() + RHSType->isBooleanType())) {
----------------
This is not especially useful: C++2a is a moving target, so p3 might mean something else later. We usually include an excerpt describing what we're checking, if it's useful to do so.
================
Comment at: lib/Sema/SemaExpr.cpp:9795
+ // C++2a [expr.spaceship]p3
+ if (int Count = (LHSType->isBooleanType() + RHSType->isBooleanType())) {
+ // TODO: The spec says that if one but not both of the operands is 'bool'
----------------
Redundant parens here.
================
Comment at: lib/Sema/SemaExpr.cpp:9807-9810
+ if (!S.Context.hasSameUnqualifiedType(LHSType, RHSType)) {
+ S.InvalidOperands(Loc, LHS, RHS);
+ return QualType();
+ }
----------------
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.
================
Comment at: lib/Sema/SemaExpr.cpp:9814-9815
+
+ LHS = S.ImpCastExprToType(LHS.get(), Type, CK_BitCast);
+ RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+ } else {
----------------
We use `CK_IntegralCast` for conversions between enumerations and integer types.
================
Comment at: lib/Sema/SemaExpr.cpp:9816
+ RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+ } else {
+ // C++2a [expr.spaceship]p4
----------------
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.
================
Comment at: lib/Sema/SemaExpr.cpp:9887-9894
// Comparisons expect an rvalue, so convert to rvalue before any
// type-related checks.
LHS = DefaultFunctionArrayLvalueConversion(LHS.get());
if (LHS.isInvalid())
return QualType();
RHS = DefaultFunctionArrayLvalueConversion(RHS.get());
if (RHS.isInvalid())
----------------
This is wrong for the three-way comparison case, where these conversions are only performed if one of the operands is of pointer type (see [expr.spaceship]p6). You could either delay decaying here, or check whether we performed a non-permitted decay after the fact.
================
Comment at: lib/Sema/SemaExpr.cpp:9925-9927
+ // C++2a [expr.spaceship]p7: If the composite pointer type is a function
+ // pointer type, a pointer-to-member type, or std::nullptr_t, the
+ // result is of type std::strong_equality
----------------
Per http://wiki.edg.com/bin/view/Wg21jacksonville2018/P0946R0-Jax18, we should also end up here if either of the operands was (a null pointer constant) of non-pointer type prior to the conversions -- that is, in the pointer comparison cases where `<` would be invalid, we should return `std::strong_equality`.
================
Comment at: lib/Sema/SemaExpr.cpp:9935-9936
+ if (CompositeTy->isPointerType() &&
+ (CompositeTy->getPointeeType()->isObjectType() ||
+ CompositeTy->getPointeeType()->isVoidType()))
+ return buildResultTy(ComparisonCategoryType::StrongOrdering);
----------------
You don't need this check; you already checked for function pointer types above, which are the only kind of non-object pointer.
================
Comment at: lib/Sema/SemaExpr.cpp:9940-9941
+ // C++2a [expr.spaceship]p9: Otherwise, the program is ill-formed.
+ // TODO: Can this case actually occur? ie we have a
+ // non-object/function/mem-function pointer, non-enum, and non-integral type
+ return InvalidOperands(Loc, LHS, RHS);
----------------
Yes, you'll get here for at least Obj-C object pointers and block pointers.
================
Comment at: lib/Sema/SemaExpr.cpp:10112
// their composite pointer type.
- if (!IsRelational &&
+ if ((!IsRelational || IsThreeWayCmp) &&
(LHSType->isMemberPointerType() || RHSType->isMemberPointerType())) {
----------------
Three-way compare is not a relational comparison, so this change appears redundant? (Likewise in other places where you use this check.) Maybe we're passing the wrong value for `IsRelational`?
================
Comment at: lib/Sema/SemaExpr.cpp:11942
ConvertHalfVec = true;
ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, true);
+ assert(ResultTy.isNull() || ResultTy->getAsCXXRecordDecl());
----------------
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`)?
https://reviews.llvm.org/D45476
More information about the cfe-commits
mailing list