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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D106784#2905919 <https://reviews.llvm.org/D106784#2905919>, @sammccall wrote:

> In D106784#2905600 <https://reviews.llvm.org/D106784#2905600>, @dblaikie wrote:
>
>> What advantage would this narrow use case have over using a raw function pointer (a function pointer member, in the case of the "S" example, for instance)?
>
> Typically you accept function_ref in an interface, and don't want to constrain callers to only passing things that can be expressed as function pointers.
>
> In the case where we found the S bug, this wasn't the case (it was just a function pointer type with better syntax). But AFAIK that's just a coincidence.

I'm not sure I follow how "that's just a coincidence" - it looks to me like the only place this could come up is a place where lambdas and other things that aren't function pointers would not be usable. So this feature could only be used in cases where the constraint is "only use function pointers, if you use anything else it'll break subtly/invoke UB". That doesn't seem like a good API design to carve out this special case while still leaving most of the uses of function_ref in the same context, UB.

I guess it comes down to: Are there situations where this patch fixes code that would've been invalid, but where the same code could use a lambda and be correct?



================
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).
----------------
sammccall wrote:
> 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.
> 
Yeah, that's basically where I am too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106784