[Sema] Simplify CheckConstraintSatisfaction. NFC

[ilya-biryukov]

Summary

• Exit early when constraint caching is disabled.
• Use unique_ptr to manage temporary lifetime.
• Fix a typo in a comment (InsertPos instead of InsertNode).

The new code duplicates the forwarding call to CheckConstraintSatisfaction,
but reduces the number of interconnected if statements and simplifies lifetime
management.

This increases the overall readability.

[ilya-biryukov]

clang/lib/Sema/SemaConcept.cpp

335

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.

[sammccall]

Thanks for cleaning up the scary new/deletes.

clang/lib/Sema/SemaConcept.cpp

321

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)

335

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?)

[ilya-biryukov]

• add FIXME for a memory leak

clang/lib/Sema/SemaConcept.cpp

321

Good point, thanks. I'll ask around to see whether the committee discussions led anywhere.

335

Oh, wow, good catch, thanks. I'll send a separate fix for the memory leak.

[ilya-biryukov]

clang/lib/Sema/SemaConcept.cpp

335

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.

[erichkeane]

In the future, when doing Concepts stuff, do you mind adding me as a reviewer? This ended up being a somewhat surprising/painful merge conflict with the deferred concepts implementation that I've been trying to get in (D119544).

[ilya-biryukov]

@erichkeane sure, will do.

clang/lib/Sema/SemaConcept.cpp

bool Sema::CheckConstraintSatisfaction(
     const NamedDecl *Template, ArrayRef<const Expr *> ConstraintExprs,
     ArrayRef<TemplateArgument> TemplateArgs, SourceRange TemplateIDRange,
     ConstraintSatisfaction &OutSatisfaction) {
   if (ConstraintExprs.empty()) {
     OutSatisfaction.IsSatisfied = true;
     return false;
   }
 
-  llvm::FoldingSetNodeID ID;
-  void *InsertPos;
-  ConstraintSatisfaction *Satisfaction = nullptr;
   bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
-  if (ShouldCache) {
-    ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
-    Satisfaction = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos);
-    if (Satisfaction) {
-      OutSatisfaction = *Satisfaction;
-      return false;
-    }
-    Satisfaction = new ConstraintSatisfaction(Template, TemplateArgs);
-  } else {
-    Satisfaction = &OutSatisfaction;
-  }
+  if (!ShouldCache) {
+    return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+                                         TemplateArgs, TemplateIDRange,
+                                         OutSatisfaction);
+  }
+  llvm::FoldingSetNodeID ID;
+  void *InsertPos;
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+    OutSatisfaction = *Cached;
+  auto Satisfaction =
+      std::make_unique<ConstraintSatisfaction>(Template, TemplateArgs);
   if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
                                     TemplateArgs, TemplateIDRange,
                                     *Satisfaction)) {
-    if (ShouldCache)
-      delete Satisfaction;
     return true;
   }
-
-  if (ShouldCache) {
-    // We cannot use InsertNode here because CheckConstraintSatisfaction might
-    // have invalidated it.
-    SatisfactionCache.InsertNode(Satisfaction);
-    OutSatisfaction = *Satisfaction;
-  }
+  // We cannot use InsertPos here because CheckConstraintSatisfaction might have
+  // invalidated it.
+  // FIXME: this leaks memory, we should allocate in the arena instead.
+  SatisfactionCache.InsertNode(Satisfaction.release());
   return false;
 }
 
 bool Sema::CheckConstraintSatisfaction(const Expr *ConstraintExpr,
                                        ConstraintSatisfaction &Satisfaction) {
   return calculateConstraintSatisfaction(
       *this, ConstraintExpr, Satisfaction,
       [](const Expr *AtomicExpr) -> ExprResult {