[PATCH] D124923: [Sema] Simplify CheckConstraintSatisfaction. NFC

[Ilya Biryukov via Phabricator via cfe-commits]

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