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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
