[llvm] r221756 - Use a function_ref now that it works (r221753).

[David Blaikie]

On Tue, Nov 11, 2014 at 6:37 PM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> Oh, I see.
>
> Would making just the member a std::function be correct? Would it be
> profitable or are we better with just a std::function everywhere?
>

I think the right answer is to use std::function everywhere here (pass by
value, use std::move, etc).

The case in Clang where we came across this was a bit narrow:

struct foo {
  function_ref f;
  foo(function_ref f) : f(f) {}
  void func() { /* stuff with 'f' */ }
};

void helper() {
  foo([&] { return ...; }).func();
}

which, once I fixed that bug, is safe - but only because the foo object was
created, used, and destroyed within a single full expression and the
underlying functor had a lifetime at least as long as the lifetime of the
'foo' object created there (both had a lifetime to the end of the full
expression).

Once you do this:

foo f([&] { return ...; });
f.func();

you would need foo to accept a std::function ctor parameter and have a
std::function member. (if you have a function_ref parameter, the
std::function would copy the function_ref, but that wouldn't copy the
underlying lambda object, so it'd have dangling pointers to the temporary
lambda object).

Hope that helps/explains things - let me know if it's at all unclear,

- David


>
> On 11 November 2014 21:28, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> > On Tue, Nov 11, 2014 at 6:23 PM, Rafael Espindola
> > <rafael.espindola at gmail.com> wrote:
> >>
> >> Author: rafael
> >> Date: Tue Nov 11 20:23:37 2014
> >> New Revision: 221756
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=221756&view=rev
> >> Log:
> >> Use a function_ref now that it works (r221753).
> >>
> >>
> >> Modified:
> >>     llvm/trunk/include/llvm/Linker/Linker.h
> >>
> >> Modified: llvm/trunk/include/llvm/Linker/Linker.h
> >> URL:
> >>
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Linker/Linker.h?rev=221756&r1=221755&r2=221756&view=diff
> >>
> >>
> ==============================================================================
> >> --- llvm/trunk/include/llvm/Linker/Linker.h (original)
> >> +++ llvm/trunk/include/llvm/Linker/Linker.h Tue Nov 11 20:23:37 2014
> >> @@ -11,8 +11,8 @@
> >>  #define LLVM_LINKER_LINKER_H
> >>
> >>  #include "llvm/ADT/SmallPtrSet.h"
> >> +#include "llvm/ADT/STLExtras.h"
> >>
> >> -#include <functional>
> >>
> >>  namespace llvm {
> >>  class DiagnosticInfo;
> >> @@ -25,7 +25,7 @@ class StructType;
> >>  /// something with it after the linking.
> >>  class Linker {
> >>    public:
> >> -    typedef std::function<void(const DiagnosticInfo &)>
> >> +    typedef function_ref<void(const DiagnosticInfo &)>
> >>          DiagnosticHandlerFunction;
> >
> >
> > I think this usage is wrong, unfortunately.
> >
> > I haven't managed to look at all the Linker code, but at least looking at
> > "Linker::Linker(Module *M)" this seems as if it'll have problems.
> >
> > That ctor initializes the member function_ref with a lambda. That' means
> the
> > function_ref will point to the lambda (unlike std::function which would
> copy
> > the lambda into itself) - but once the variable's initializer in the ctor
> > finishes, the lambda will be destroyed, leaving the function_ref
> dangling, I
> > think?
> >
> > Any other caller that does something like:
> >
> > Linken L([] { ... });
> > L.functionThatUsesTheMemberFunctionRef();
> >
> > Will be equally problematic - as the lambda will be destroyed at the end
> of
> > the Linker variable's initialization, and the member function_ref will be
> > left dangling.
> >
> >>
> >>
> >>      Linker(Module *M, DiagnosticHandlerFunction DiagnosticHandler);
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141111/f6f316a8/attachment.html>