ilya-biryukov added inline comments.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:321
+ bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+ if (!ShouldCache) {
----------------
sammccall wrote:
> 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)
Good point, thanks. I'll ask around to see whether the committee discussions
led anywhere.
================
Comment at: clang/lib/Sema/SemaConcept.cpp:335
+ auto Satisfaction =
+ std::make_unique<ConstraintSatisfaction>(Template, TemplateArgs);
if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
----------------
sammccall wrote:
> 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?)
Oh, wow, good catch, thanks. I'll send a separate fix for the memory leak.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124923/new/
https://reviews.llvm.org/D124923
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
