[PATCH] D106784: [ADT] function_ref captures function pointers by value

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

Seems like there's 3 alternatives at this point:

- support function pointers by value (this patch)
- disallow function pointers/references entirely (breaks existing code)
- function pointers are UB if the pointer dies, work around the GCC bug somehow (std::function_ref takes this path)



================
Comment at: llvm/include/llvm/ADT/STLExtras.h:168-170
+///
+/// (As an exception, function_ref(&someFreeFunction) is safe, even though the
+/// callable is technically a temporary function pointer).
----------------
chandlerc wrote:
> The more I think about this the more I feel like we shouldn't try to make this promise... it seems too narrow to be useful. I think maybe if we want to do something here, causing things to break immediately rather than kinda-sorta working is better.
> 
> For the code where this came up, I think just moving away from `function_ref` is a much better approach than relying on the special behavior in the case of a pointer.
> 
> But happy to defer to dblaikie here ultimately.
> it seems too narrow to be useful

I think it's useful - it fixes broken code people expect to write e.g. [this looks like an existing bug](https://github.com/llvm/llvm-project/blob/1930c4410d6b48645b7b7c58cf4403a2a0e3836d/llvm/include/llvm/Transforms/Utils/SimplifyLibCalls.h#L144). And I'm not sure that making it broader would actually be more useful! Nobody expects to be able to *store* e.g. a temporary lambda in a function_ref, but I think that expectation exists for function pointers.

However FWIW on the standard track it's not going to be supported for consistency reasons: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p0792r5.html#lifetime-of-pointers-to-function (missed this on my first scan).

> For the code where this came up, I think just moving away from function_ref is a much better approach

Agreed, I've committed such a fix to make sure we have something in time for the 13 branch.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106784