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

2022-05-05 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added a comment.

@erichkeane sure, will do.


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

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

2022-05-05 Thread Erich Keane via Phabricator via cfe-commits 
erichkeane added a comment.

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


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

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

2022-05-04 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:335
+  auto Satisfaction =
+  std::make_unique(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

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

2022-05-04 Thread Ilya Biryukov via Phabricator via cfe-commits 
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG726d7b07fcde: [Sema] Simplify CheckConstraintSatisfaction. 
NFC (authored by ilya-biryukov).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124923

Files:
  clang/lib/Sema/SemaConcept.cpp


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -318,35 +318,31 @@
 return false;
   }
 
+  bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+  if (!ShouldCache) {
+return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+ TemplateArgs, TemplateIDRange,
+ OutSatisfaction);
+  }
   llvm::FoldingSetNodeID ID;
+  ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
   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 = 
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+OutSatisfaction = *Cached;
+return false;
   }
+  auto Satisfaction =
+  std::make_unique(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;
-  }
+  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;
 }
 


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -318,35 +318,31 @@
 return false;
   }
 
+  bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+  if (!ShouldCache) {
+return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+ TemplateArgs, TemplateIDRange,
+ OutSatisfaction);
+  }
   llvm::FoldingSetNodeID ID;
+  ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
   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 = 
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+OutSatisfaction = *Cached;
+return false;
   }
+  auto Satisfaction =
+  std::make_unique(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;
-  }
+  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;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-04 Thread 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(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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-04 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov updated this revision to Diff 427018.
ilya-biryukov added a comment.

- add FIXME for a memory leak


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124923

Files:
  clang/lib/Sema/SemaConcept.cpp


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -318,35 +318,31 @@
 return false;
   }
 
+  bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+  if (!ShouldCache) {
+return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+ TemplateArgs, TemplateIDRange,
+ OutSatisfaction);
+  }
   llvm::FoldingSetNodeID ID;
+  ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
   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 = 
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+OutSatisfaction = *Cached;
+return false;
   }
+  auto Satisfaction =
+  std::make_unique(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;
-  }
+  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;
 }
 


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -318,35 +318,31 @@
 return false;
   }
 
+  bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+  if (!ShouldCache) {
+return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+ TemplateArgs, TemplateIDRange,
+ OutSatisfaction);
+  }
   llvm::FoldingSetNodeID ID;
+  ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
   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 = 
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+OutSatisfaction = *Cached;
+return false;
   }
+  auto Satisfaction =
+  std::make_unique(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;
-  }
+  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;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-04 Thread 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(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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2022-05-04 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov added inline comments.



Comment at: clang/lib/Sema/SemaConcept.cpp:335
+  auto Satisfaction =
+  std::make_unique(Template, TemplateArgs);
   if (::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,

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.


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

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

2022-05-04 Thread Ilya Biryukov via Phabricator via cfe-commits 
ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, saar.raz.
Herald added a project: All.
ilya-biryukov requested review of this revision.
Herald added a project: clang.

- 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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D124923

Files:
  clang/lib/Sema/SemaConcept.cpp


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -318,35 +318,30 @@
 return false;
   }
 
+  bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+  if (!ShouldCache) {
+return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+ TemplateArgs, TemplateIDRange,
+ OutSatisfaction);
+  }
   llvm::FoldingSetNodeID ID;
+  ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
   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 = 
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+OutSatisfaction = *Cached;
+return false;
   }
+  auto Satisfaction =
+  std::make_unique(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;
-  }
+  OutSatisfaction = *Satisfaction;
+  // We cannot use InsertPos here because CheckConstraintSatisfaction might 
have
+  // invalidated it.
+  SatisfactionCache.InsertNode(Satisfaction.release());
   return false;
 }
 


Index: clang/lib/Sema/SemaConcept.cpp
===
--- clang/lib/Sema/SemaConcept.cpp
+++ clang/lib/Sema/SemaConcept.cpp
@@ -318,35 +318,30 @@
 return false;
   }
 
+  bool ShouldCache = LangOpts.ConceptSatisfactionCaching && Template;
+  if (!ShouldCache) {
+return ::CheckConstraintSatisfaction(*this, Template, ConstraintExprs,
+ TemplateArgs, TemplateIDRange,
+ OutSatisfaction);
+  }
   llvm::FoldingSetNodeID ID;
+  ConstraintSatisfaction::Profile(ID, Context, Template, TemplateArgs);
   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 = 
+  if (auto *Cached = SatisfactionCache.FindNodeOrInsertPos(ID, InsertPos)) {
+OutSatisfaction = *Cached;
+return false;
   }
+  auto Satisfaction =
+  std::make_unique(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;
-  }
+  OutSatisfaction = *Satisfaction;
+  // We cannot use InsertPos here because CheckConstraintSatisfaction might have
+  // invalidated it.
+  SatisfactionCache.InsertNode(Satisfaction.release());
   return false;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits