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