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/
+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/
+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.)

cfe-commits mailing list

Reply via email to