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

[Sam McCall via Phabricator via cfe-commits]

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