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

[Eric Fiselier via Phabricator via cfe-commits]

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


================
Comment at: include/clang/AST/ComparisonCategories.h:68
+  /// \brief A map containing the comparison category values built from the
+  /// standard library. The key is a value of ComparisonCategoryValue.
+  llvm::DenseMap<char, DeclRefExpr *> Objects;
----------------
rsmith wrote:
> What is ComparisonCategoryValue? Do you mean ComparisonCategoryResult?
Yes. Fixed.


================
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:
> 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?


================
Comment at: lib/AST/ExprConstant.cpp:7107
   IntExprEvaluator(EvalInfo &info, APValue &result)
-    : ExprEvaluatorBaseTy(info), Result(result) {}
+      : ExprEvaluatorBase(info), Result(result) {}
 
----------------
rsmith wrote:
> Historically MSVC has choked on use of the injected-class-name of a base class that is a template specialization. Is this fixed in our supported versions now?
No idea. I didn't mean to change it (I had a different version that did).

I'll correct it.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8888
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;
----------------
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?


https://reviews.llvm.org/D45476