[llvm-branch-commits] [clang] [NFC][SSAF] Extract common code in Analyses to a shared file (PR #191932)

[Balázs Benics via llvm-branch-commits]

================
@@ -86,19 +68,19 @@ class EntityPointerLevelTranslator
     return EntityPointerLevelSet{Incremented.begin(), Incremented.end()};
   }
 
-  std::function<EntityId(EntityName EN)> AddEntity;
+  llvm::function_ref<EntityId(EntityName EN)> AddEntity;
   ASTContext &Ctx;
 
 public:
-  EntityPointerLevelTranslator(std::function<EntityId(EntityName EN)> AddEntity,
-                               ASTContext &Ctx)
+  EntityPointerLevelTranslator(
+      llvm::function_ref<EntityId(EntityName EN)> AddEntity, ASTContext &Ctx)
       : AddEntity(AddEntity), Ctx(Ctx) {}
----------------
steakhal wrote:

I think I left once the review feedback that one should always prefer `llvm::function_ref` over `std::function` (one is a borrow, while the other is an owning type).
While this is kindof true for regular function which do their stuff and then return and usually don't "store" the acquired fn refs; constructors are different.
In their case their callsite does not outlive the lifetime of the "*this" object; thus we will keep referring to the fn we pass.
If that is a lambda, then we will be in trouble.

If I left this review comment exactly here, then it's my bad. I was probably overloaded and overlooked this.

---

Anyway, all in all, what we had here, taking `std::function` is actually the right thing to do - except that when we forward it to initialize `AddEntity` non-static data member, we should have cast it to `std::move` first - again, to avoid a copy of the owning non-trivial `std::function` object.

https://github.com/llvm/llvm-project/pull/191932