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:
> > 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.
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8888
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+ using CCKT = ComparisonCategoryKind;
----------------
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.
https://reviews.llvm.org/D45476
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
