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?

cfe-commits mailing list

Reply via email to