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

Wed May 4 10:22:50 PDT 2022

ilya-biryukov added inline comments.

================
Comment at: clang/lib/Sema/SemaConcept.cpp:335
+  auto Satisfaction =
+      std::make_unique<ConstraintSatisfaction>(Template, TemplateArgs);
   if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
----------------
ilya-biryukov wrote:
> 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.
So there's no memory leak actually.
`Sema`'s destructor calls `delete` for all nodes inside `SatisfactionCache`. Allocating them in `ASTContext` is not trivial as they store a `SmallVector`, which may dynamically allocate itself. Hence, we must call destructor and `ASTContext` does not do that. 
The `SmallVector` itself gets built recursively when calling `CheckConstraintSatisfaction`.

Cleaning it up is probably possible, but a bit more work than I planned for this. I'll replace the fixme with a comment mentioning `Sema` destructor cleans up for us.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124923/new/

https://reviews.llvm.org/D124923