Missing reference in function template?

This works in Visual C++ 2010:

Code:

#include <boost/noncopyable.hpp>
#include <stdio.h>
#include <functional>
#include <utility>

using namespace std;

template <typename T>
class scoped_sentry: boost::noncopyable
{
    T& object;
    function<void(T&)> cleanup;
public:
    scoped_sentry(T&& object, function<void(T&)> cleanup)
        : object(object), cleanup(cleanup) {}
    ~scoped_sentry() { cleanup(object); }
    operator T&() { return object; }
};

int main()
{
    scoped_sentry<FILE*> in(
        fopen("test.txt", "r"), 
        [](FILE* fp) { if (fp) fclose(fp); });
    int ch;

    while ((ch = fgetc(in)) != EOF)
        putchar(ch);
}

My concern is the lambda argument of FILE* does not match the expected type of the cleanup function template, FILE*& after argument expansion. But it works! Is this safe, or do I need to use FILE*& in the lambda?

Compiled with G++ 4.6:

Code:

g++ -std=c++0x -Wall -pedantic main.cpp -o main -g

Your programs ends with a segmentation fault:

Code:

GNU gdb (GDB) 7.2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-unknown-linux-gnu".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Reading symbols from /home/florian/code/cpp/cboard/138402/main...done.
(gdb) run
Starting program: /home/florian/code/cpp/cboard/138402/main

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff7351c11 in getc () from /lib/libc.so.6
(gdb) bt
#0  0x00007ffff7351c11 in getc () from /lib/libc.so.6
#1  0x0000000000400921 in main () at main.cpp:27
(gdb) exit

Unfortunately, i cant figure out why.

It works on MinGW and I don't have a Linux machine to test. Does a straight FILE* work?

Code:

int main()
{
    FILE* in = fopen("test.txt", "r");
    int ch;

    while ((ch = fgetc(in)) != EOF)
        putchar(ch);
}

Just because code compiles does not make it safe.

Your code stores a reference to a return value from fopen() - which happens to be a pointer. As such, there is - depending on how the compiler manages lifetime of (temporary) values returned from functions - potential of a dangling reference. Any use of that dangling reference gives undefined behaviour.

Probably easier to make object (in scoped_sentry) a pointer rather than a reference, and adjust other things accordingly. That might reduce your ability to use move semantics, but the benefit of that is minimal in your case anyway.

Just because code compiles does not make it safe.

Duh, that's why I asked if it was safe.

Your code stores a reference to a return value from fopen() - which happens to be a pointer. As such, there is - depending on how the compiler manages lifetime of (temporary) values returned from functions - potential of a dangling reference. Any use of that dangling reference gives undefined behaviour.

Can you give a realistic example where there could be a dangling reference? As I understand it, binding the temporary to a reference extends the lifetime of the temporary, or a copy of the temporary, for the scope of the reference. That scope is the lifetime of the scoped_sentry object. Is there something going on between the reference and the rvalue reference that would cause a dangling reference?

The lifetime of a pointer is a completely separate deal than the lifetime of the thing pointed to -- which is why extending the lifetime of a pointer is often a not-helpful thing to do.

In this particular case, since the pointer came from fopen, then I think it should be good until you hit fclose (assuming it was good in the first place -- i.e., assuming the file actually opens).

O_o

The `FILE*&' is syntax goodness to refer to what is internally almost certainly a `FILE**' by the name of the reference directly instead of dereferencing a pointer. In other words, a `FILE*&' is naturally a `FILE*' in the same way that a 'FILE**' is a `FILE*' when dereferenced. This is simple and required behavior. There is nothing wrong with it.

Binding a `rvalue' from a simple temporary to a constant reference, a mutable `rvalue' reference, and a constant `rvalue' reference are all well defined and have required and guaranteed semantics by the standard. This is relevant to the template but not this use. It is possible, by misusing operator chaining, that your source will do the wrong thing in the face of some `rvalues'. Realistically, you will want to overload the constructor to create a temporary object for containment, as member data, that will do the correct thing.

Soma

The lifetime of a pointer is a completely separate deal than the lifetime of the thing pointed to -- which is why extending the lifetime of a pointer is often a not-helpful thing to do.

I am not worried about the thing pointed to, just a type mismatch in the lambda and now the potential dangling reference grumpy was talking about.

The `FILE*&' is syntax goodness to refer to what is internally almost certainly a `FILE**' by the name of the reference directly instead of dereferencing a pointer. In other words, a `FILE*&' is naturally a `FILE*' in the same way that a 'FILE**' is a `FILE*' when dereferenced. This is simple and required behavior. There is nothing wrong with it.

But FILE* and FILE*& are different types and not compatible. If I take the function template away g++ throws errors:

Code:

#include <stdio.h>

void foo(FILE* fp) {}
void test(void(*pf)(FILE*&)) {}

int main()
{
    // error: invalid conversion from 'void (*)(FILE*)' to 'void (*)(FILE*&)' [-fpermissive]
    test(foo);

    // error: cannot convert 'main()::<lambda(FILE*)>' to 'void (*)(FILE*&)' for argument '1' to 'void test(void (*)(FILE*&))'
    test([](FILE* fp) {});
}

My first question is why does this assignment work when bound to a function template and is it safe? Is the function template hiding a bug or is it silently doing the "right thing", whatever that is?

Code:

#include <stdio.h>
#include <functional>

using namespace std;

void foo(FILE* fp) {}
void test(function<void(FILE*&)>) {}

int main()
{
    // Compiles without errors!
    test(foo);
    test([](FILE* fp) {});
}

Binding a `rvalue' from a simple temporary to a constant reference, a mutable `rvalue' reference, and a constant `rvalue' reference are all well defined and have required and guaranteed semantics by the standard. This is relevant to the template but not this use. It is possible, by misusing operator chaining, that your source will do the wrong thing in the face of some `rvalues'. Realistically, you will want to overload the constructor to create a temporary object for containment, as member data, that will do the correct thing.

I think the intention of scoped_sentry is obvious, but what I want to do is generically add RAII to an unmanaged resource. In this code it's FILE* where fclose() needs to be called at the end. Before rvalue references I would do it with an lvalue reference and the wrapping step is manual:

Code:

template <typename T>
class scoped_sentry: boost::noncopyable
{
    T& object;
    function<void(T&)> cleanup;
public:
    scoped_sentry(T& object, function<void(T&)> cleanup)
        : object(object), cleanup(cleanup) {}
    ~scoped_sentry() { cleanup(object); }
};

int main()
{
    FILE* in = fopen("test.txt", "r");
    scoped_sentry<FILE*> sentry(
        in,
        [](FILE* fp) { if (fp) fclose(fp); });
    int ch;

    while ((ch = fgetc(in)) != EOF)
        putchar(ch);
}

With rvalue references, I thought I could bring the RAII wrapper closer to the unmanaged resource so that initialization is direct in the constructor and the scoped_sentry object replaces the wrapped object. That way a "dummy" object wouldn't be needed and scoped_sentry could manage the resource completely:

Code:

int main()
{
    scoped_sentry<FILE*> in(
        fopen("test.txt", "r"),
        [](FILE* fp) { if (fp) fclose(fp); });
    int ch;

    while ((ch = fgetc(in)) != EOF)
        putchar(ch);
}

I feel better about taking away the reference member and owning a copy, where the rvalue reference in the constructor keeps the semantics like I want without excessive copying. But I am not confident that this will work generically for any T. I can not think of any cases where an unmanaged resource would be represented by anything except a handle, but I am not sure enough about that to see this as complete:

Code:

template <typename T>
class scoped_sentry: boost::noncopyable
{
    T object;
    function<void(T)> cleanup;
public:
    scoped_sentry(T&& object, function<void(T)> cleanup)
        : object(object), cleanup(cleanup) {}
    ~scoped_sentry() { cleanup(object); }
    operator T&() { return object; }
};

What do you think?

I think you should just document that it works for copyable types T. Saying that there are no unmanaged resources that can't be copied is a devil's proof.

I am not worried about the thing pointed to, just a type mismatch in the lambda and now the potential dangling reference grumpy was talking about.

Well, I'm not grumpy. But anyway, you've assigned a bound temporary, the named `rvalue' reference parameter, to a variable that outlives the scope of the variable binding that temporary.

But FILE* and FILE*& are different types and not compatible.

Yes, they are compatible.

It is the function pointers that aren't compatible, but then, you've misunderstood what is happening to that pointer behind the scenes.

I think the intention of scoped_sentry is obvious, but what I want to do is generically add RAII to an unmanaged resource.

The intent is obvious. That isn't even relevant to the comment you quoted.

With rvalue references, I thought I could bring the RAII wrapper closer to the unmanaged resource so that initialization is direct in the constructor and the scoped_sentry object replaces the wrapped object.

This is possible, but you still have to use all references correctly and design your constructor to do the "right thing" in the face of a temporary that you can destroy. That is what `rvalue' references were born to do. As I've said, this is relevant to your template, but not this example use.

The example here is only a simple pointer. (It is only generated as a `rvalue'.) You should to overload the constructor, or use indirection through an extra template, to craft an object that can contain that pointer by a means that guarantees the life of that pointer until the end of the object containing that pointer. It isn't strictly necessary, but it actually does simplify the entire class.

You must overload the constructor, or use indirection through an extra template, for the constructor to do the "best thing" in the face of an `rvalue' reference. Here, this use, it doesn't matter. However, in the face of an `rvalue' temporary that is expensive to copy, you can attempt constructs to destroy that temporary in assigning it to a variable that is guaranteed to live as long as the object that contains that variable. This only works if the object in question supports such destructive semantics, but that is why `rvalue' references. They are not a magic bit of semantics that increase the life of temporaries any more than an old fashion constant reference allowed.

I feel better about taking away the reference member and owning a copy, where the rvalue reference in the constructor keeps the semantics like I want without excessive copying.

This is generally the correct approach. You can now use mechanisms to determine the best approach in keeping the value alive without ........ing about trying to keep a variable alive.

Here is the interesting part, if a creation function, such as a factory or convenience function, returns an object "by value", instead of a handle to that value, it returns a copy. The only way for such an object to work is to have a copy constructor. (It doesn't need an assignment operator.) The alternative is having the creation function return an object that can't be copied to be used as a package by the constructor of the target object which itself can not be copied. (For example, neither class `A' nor `B' has copy constructors and the creation routine packages all that is necessary for `A' to be constructed into a `B' object which `A' consumes as part of construction.) Happily, because you are now containing an instance of the class, you can do the correct thing in any event by making the constructor a template and simply attempting to construct the relevant type with that parameter in exactly the same way you have called the copy constructor.

Soma

It is the function pointers that aren't compatible, but then, you've misunderstood what is happening to that pointer behind the scenes.

Please explain my misunderstanding then, because I still do not get what the difference is.

You should to overload the constructor, or use indirection through an extra template, to craft an object that can contain that pointer by a means that guarantees the life of that pointer until the end of the object containing that pointer. It isn't strictly necessary, but it actually does simplify the entire class.

You must overload the constructor, or use indirection through an extra template, for the constructor to do the "best thing" in the face of an `rvalue' reference. Here, this use, it doesn't matter. However, in the face of an `rvalue' temporary that is expensive to copy, you can attempt constructs to destroy that temporary in assigning it to a variable that is guaranteed to live as long as the object that contains that variable. This only works if the object in question supports such destructive semantics, but that is why `rvalue' references. They are not a magic bit of semantics that increase the life of temporaries any more than an old fashion constant reference allowed.

I am sorry, but this confuses me just as much as grumpy's dangling reference that I do not see. Can you show me an example of what this overloaded constructor is supposed to look like and what these "constructs" are?

You can now use mechanisms to determine the best approach in keeping the value alive without ........ing about trying to keep a variable alive.

Mechanisms...such as? If I understood these "mechanisms", I would have used them in the first place. Please help me learn.

FILE* and FILE*& are compatible, since you can do:

Code:

void foo(FILE*&) {}
FILE* f;
foo(f);

The reason why your example doesn't work is because the function signatures are different. Obviously,

Code:

void foo(FILE*);
void foo(FILE*&);

do very different things. Hence, you cannot assign the address of one such function to a function pointer of the other type.

Also, your vanilla shared pointers have the option of a custom deleter, which you can use to clean up unmanaged resources. Then there's also Boost::ScopeExit. Just a little FYI.

Please explain my misunderstanding then, because I still do not get what the difference is.

Basically, `std::function<???>' is doing more behind the scenes than simple copying the function pointer exactly as you may have intended. It is, in a way, polymorphism at play.

I am sorry, but this confuses me just as much as grumpy's dangling reference that I do not see.

You have bound a temporary to a `rvalue' reference (the constructor's function parameter). The temporary is guaranteed to live as long as the variable that names that reference. You however have assigned that value to another variable (the class data member) that lives beyond that binding variable.

If I understood these "mechanisms", I would have used them in the first place.

In the case of a handle, you only need to copy the handle. In the case of an object, you need to copy the object.

With the use of `rvalue' references and type information gleaned from the template you can discover with which case you are operating.

Further, you can use `std::move' and the potential of "move overloads" for the constructor of the target object to perform a destructive move on the contents of the object passed to your constructor.

Finally, you can go the extra step of allowing construction of the contained object by proxy by simply forwarding that data the same as you would have forwarded an object to a copy constructor.

If you don't want to mess with any of this, or don't understand it, just deal with keeping a copy for now and read up on what `rvalue' references actually do. There is a splendid example in the errata for the original proposal for the feature.

Soma

The reason why your example doesn't work is because the function signatures are different.

Yes, but why is the lambda signature's FILE* compatible with the function template's FILE*& when using a function instead of a function template is not? That was my question. It should fail to compile if the signatures do not match, but two C++0x compilers compile it without any warnings or errors.

It looks like the function template is polymorphic...however that is supposed to work.

Also, your vanilla shared pointers have the option of a custom deleter, which you can use to clean up unmanaged resources.

My first attempt was with unique_ptr.

Code:

#include <stdio.h>
#include <memory>

using namespace std;

int main()
{
    struct fclose_deleter
    {
        void operator()(FILE* fp) { if (fp) fclose(fp); }
    };

    unique_ptr<FILE, fclose_deleter> in(fopen("test.txt", "r"));
    int ch;

    while ((ch = fgetc(in.get())) != EOF)
        putchar(ch);
}

The are two problems that brought me to my own RAII wrapper:

• A direct lambda can not be used in place of fclose_deleter
  
  Code:

  // Will not compile!
  unique_ptr<FILE, [](FILE* fp) { if (fp) fclose(fp); }> in(fopen("test.txt", "r"));

  I wanted something as self-contained as possible, and a separate function, function object, or stored lambda is not quite there.
• To get the FILE* from a unique_ptr, I needed to call get(). I wanted the RAII wrapper to sit in place of the unmanaged resource without any extra syntax to use it.

Boost::ScopeExit did not even enter the running because it is ugly as sin.

Basically, `std::function<???>' is doing more behind the scenes than simple copying the function pointer exactly as you may have intended. It is, in a way, polymorphism at play.

Thank you. I will do more research on it.

You have bound a temporary to a `rvalue' reference (the constructor's function parameter). The temporary is guaranteed to live as long as the variable that names that reference. You however have assigned that value to another variable (the class data member) that lives beyond that binding variable.

I see now, thank you.

If you don't want to mess with any of this, or don't understand it, just deal with keeping a copy for now and read up on what `rvalue' references actually do. There is a splendid example in the errata for the original proposal for the feature.

I've read that and other articles many times. Rvalue references are too subtle for my little brain.

Code:

unique_ptr<FILE, [](FILE* fp) { if (fp) fclose(fp); }> in(fopen("test.txt", "r"));

Um... what? No. Very, very no.

Code:

unique_ptr<FILE, function<void(FILE*)> > in(fopen("test.cxx", "r"), ([](FILE * f){if(f){fclose(f);}}));

Soma