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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
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 &&
----------------
EricWF wrote:
> rsmith wrote:
> > 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.
> I'm confused. This does deal in `DeclRefExpr*`s. I'm probably being daft. Could you clarify?
Sorry, while editing this comment I managed to reverse it from what I meant. This should deal in NamedDecl* (or perhaps ValueDecl*) , not DeclRefExpr*.


================
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<
----------------
EricWF wrote:
> rsmith wrote:
> > Why are you diagnosing this case as an error?
> [expr.spaceship]p9 seems to require it. The composite pointer type is not a function/member/null pointer type, neither is it a object pointer type, so the program is ill-formed.
> 
> Unless I'm missing something.
void* is an object pointer type.


================
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.
----------------
EricWF wrote:
> rsmith wrote:
> > 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.
> I'm struggling to answer that question myself. The case I was thinking of that I wanted to help the user out with is:
> 
> ```
> auto cmp_sentinal(long val) {
>   int SENTINAL = 0;
>   return SENTINAL <=> val; // error, would be OK if SENTINAL were const.
> }
> ```
> 
> I'll remove these diagnostics for now, and hopefully improve them in a follow up patch, if that's OK with you?
Sounds good.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8888
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;
----------------
EricWF wrote:
> rsmith wrote:
> > 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.
> I agree that it's preferable to just have a cache. However can you clarify what you mean by "build it in ASTContext"?
> 
> Building the expressions will potentially require emitting a diagnostic, and use of bits of `Sema` (like name lookup).
> I'm not sure how to do this inside `ASTContext`.
> 
> The current implementation caches the data in `ASTContext` but builds it as-needed in `Sema`. Could you clarify how to go about with the implementation you're suggesting?
You don't need "real" name lookup here, just calling lookup on the DeclContext should be fine. We don't need to support more exotic ways of declaring these members, nor access checks etc.

I was imagining that Sema would check that we can find suitable members on the comparison category types as part of semantically checking a <=> expression, and the ASTContext side of things would not emit diagnostics.

(Put another way, we'd act as if we look up the members each time we need them, Sema would check that all the necessary members actually exist, and ASTContext would have a caching layer only for convenience / to avoid repeatedly doing the same lookups.)


https://reviews.llvm.org/D45476