sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Thanks for cleaning up the scary new/deletes.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:321
+ bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+ if (!ShouldCache) {
----------------
Another loose end that could be cleaned up sometime!
`ConceptSatisfactionCaching` is on by default and controlled by an -f flag.
It was added in https://reviews.llvm.org/D72552 - essential for performance,
unclear whether the caching scheme was allowed.
Probably this option should be removed (and if needed, the caching scheme
semantics made to match what was standardized)
================
Comment at: clang/lib/Sema/SemaConcept.cpp:335
+ auto Satisfaction =
+ std::make_unique<ConstraintSatisfaction>(Template, TemplateArgs);
if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
----------------
ilya-biryukov wrote:
> I also wonder if this could be allocated in the AST arena, maybe worth
> exploring.
> Definitely outside this change, though, would like to keep this one an NFC.
not just could, but must - FoldingSet doesn't own its objects, so this object
is leaked once the FoldingSet is destroyed! Allocating it on the ASTContext
would fix this because they have the same lifetime.
Agree with not mixing it into this patch, but please add a FIXME and do follow
up on it if you don't mind.
(and at that point, should we really be passing it back by value?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124923/new/
https://reviews.llvm.org/D124923
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
