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

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 30 inline comments as done.
EricWF added inline comments.


================
Comment at: include/clang/AST/ComparisonCategories.h:194
+
+  const ASTContext &Ctx;
+  mutable llvm::DenseMap<char, ComparisonCategoryInfo> Data;
----------------
Storing the `ASTContext` in `ComparisonCategories` and `ComparisonCategoryInfo` is a bit of a hack. I'm going to work on either passing `ASTContext`  as needed, or integrating these types directly with `ASTContext`


================
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 &&
----------------
rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > 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*.
> > OK, so I tried changing this to `VarDecl` but it made the `ExprConstant` and `GCExprAgg` implementations a lot harder, since we actually want to evaluate the result as a `DeclRefExpr` as we're not evaluating the actual `Decl`. 
> > 
> > Since we don't want to be building dummy `DeclRefExpr`s during evaluation just to throw them away, I think it makes sense to eagerly build the results as `DeclRefExpr`s.
> > 
> > Does that make sense?
> Not really; forming a value that denotes an arbitrary (non-reference) variable in both those places seems like it should be straightforward. If you really want a DeclRefExpr in those places as an implementation convenience, you could synthesize one on the stack. But that'd be an implementation detail of those consumers of this information, not something we should be exposing here, and seems like it should be easy to avoid.
Ack. Done. I'm not sure why CodeGen gave me so much trouble. But you were right, it ended up being straight forward.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8888
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;
----------------
rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > 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.)
> > That's pretty much how this currently works, right?
> > 
> > The only difference is that Sema still eagerly builds the potential result values eagerly, because they'll be needed later.
> You can't rely on Sema existing or having done this in the case where we're loading from an AST file.
Ack. Thanks for sticking with me. I understand now.


https://reviews.llvm.org/D45476