Std::Unique_Ptr for C Functions That Need Free

std::unique_ptr for C functions that need free

What you have is extremely likely to work in practice, but not strictly correct. You can make it even more likely to work as follows:

std::unique_ptr<char, decltype(std::free) *>
    t_copy { strdup(t), std::free };

The reason is that the function type of std::free is not guaranteed to be void(void*). It is guaranteed to be callable when passed a void*, and in that case to return void, but there are at least two function types that match that specification: one with C linkage, and one with C++ linkage. Most compilers pay no attention to that, but for correctness, you should avoid making assumptions about it.

However, even then, this is not strictly correct. As pointed out by @PeterSom in the comments, C++ allows implementations to e.g. make std::free an overloaded function, in which case both your and my use of std::free would be ambiguous. This is not a specific permission granted for std::free, it's a permission granted for pretty much any standard library function. To avoid this problem, a custom function or functor (as in his answer) is required.

Assuming it makes sense, it is possible to use a similar pattern with non-pointers?

Not with unique_ptr, which is really specific to pointers. But you could create your own class, similar to unique_ptr, but without making assumptions about the object being wrapped.

Can I succintly declare std::unique_ptr with custom deleter?

Let the language do the hard work!

#include <memory>

struct qdr_link_t;
qdr_link_t* new_qdr_link_t();
void free_qdr_link_t(qdr_link_t*);

template <typename T, typename Deleter>
auto make_unique_ptr(T* raw, Deleter deleter)
{
    return std::unique_ptr<T, Deleter>(raw, deleter);
}

//std::unique_ptr<qdr_link_t, decltype(&free_qdr_link_t)> link{new_qdr_link_t(), free_qdr_link_t};
auto link = make_unique_ptr(new_qdr_link_t(), free_qdr_link_t);

Add std::forward to taste (if you care).

---

For C++11, you'll need to add the trailing return type -> std::unique_ptr<T, Deleter> to make_unique_ptr, or just put that in the "normal" return type.

If a function returns a std::unique_ptr, is it reasonable to assume each returned value is to a new object?

It should be assumed that, if a function returns std::unique_ptr<T>, then the returned smart pointer points to an object that is not currently managed by anyone else. This does not necessarily mean that it always refers to a different object. So long as this convention is followed, double-free bugs will be avoided. If this convention is violated, double-free bugs will occur.

For example, if you see some function like this:

std::unique_ptr<T> foo(std::unique_ptr<T> arg);

This function might, potentially, return std::move(arg) under some circumstances or it might destroy arg and return some other pointer. (You have to read the documentation to know what it does). This implies that you could do something like this:

auto t = std::make_unique<T>();
t = foo(std::move(t));
t = foo(std::move(t));

In this case, foo might just return the same pointer value twice, and this is perfectly safe. This example seems silly, but hopefully it gets my point across.

Convert unique_ptr to void* and back, do I need to free?

std::unique_ptr<Task> taskPtr(new Task(23));

When this variable goes out of scope, the dynamic allocation will be deleted. There is no need to, and you mustn't delete it yourself unless you call release on the unique pointer, which is the only way to transfer the ownership out of a unique pointer (besides transferring ownership to another unique pointer).

Any casts that you may do have no effect on this.

C++: Wrapping C style array into unique_ptr leads to double free or corruption

int arr[] = {1, 2, 3, 4, 5, 6, 7, 8};

This variable has automatic storage duration. It will be destroyed at the end of the scope where it is declared.

You pass a pointer to the first element of the automatic array into the constructor. The constructor initialises the unique pointer member to point to the automatic array. In the destructor, the unique pointer is destroyed and its destructor invokes delete[] on the stored pointer. Invoking delete[] on any pointer that was not acquired through new of an array results in undefined behaviour. The pointer in this case points to an automatic array which was not acquired by new[], and therefore the behaviour of the program is undefined.

As far as I understand, the only place where arr is destroyed, should be the destructor of std::unique_ptr

That is not where arr may be destroyed, because arr is an automatic variable.

In conclusion: (Almost) never store pointers to any variables in smart pointers. Only store pointers to dynamic objects, whose deallocation function matches the deleter of the smart pointer. Prefer to create smart pointers using std::make_unique (and related functions) when possible to avoid making this mistake in the future.

---

I go through them and wrap them into unique_ptr, but I'm not sure how they were allocated.

These two things are not compatible with one another. In order to use a smart pointer, you must know how something is allocated because otherwise you cannot know the corresponding deleter to use with the smart pointer.

I have a C function

C API usually have a pair of allocation and deallocation functions, or they use malloc and expect you to free. The relevant details should be in the documentation of the API.

Do I need to use unique_ptr over std::function

std::function uses some form of type erasure, therefore, it itself shouldn't be too big. For instance, in my experiment with GCC and libstdc++, all instances of std::function I tried had 32 bytes: live demo. Therefore, moving std::function objects should be relatively cheap (copying might be a different thing).

Anyway, std::unique_ptr is still smaller (typically a size of a raw pointer), therefore, it will be moved faster at an assembly level. Whether it does matter is a matter of profiling.

Why does std::unique_ptr.release() get evaluated before member function access which is in the lhs of assignment?

This is because of [expr.ass]/1 which states:

[...] In all cases, the assignment is sequenced after the value computation of the right and left operands, and before the value computation of the assignment expression. The right operand is sequenced before the left operand. With respect to an indeterminately-sequenced function call, the operation of a compound assignment is a single evaluation.

^{emphasis mine}

According to the above the right hand side gets evaluated first, then the left hand side, then the assignment happens. It should be noted that this is only guaranteed since C++17. Before C++17 the order of the left and right sides evaluations was unspecified.

Use of unique_ptr for class members

There is nothing technically wrong with your use of std::unique_ptr here, but it's totally unnecessary in this situation. std::vector is already essentially a unique pointer to an underlying array. Instead of having your class have a std::unique_ptr<std::vector<char>> member it should likely just have a std::vector<char> member.

---

I would also question why load_image and save_image are static if they need references/pointers to the class's members (or the object itself, in the case of save_image). It seems like it would make much more sense for them to be non-static member functions. That way they would implicitly have access to the members of the object on which they're called.

There's also a potential memory leak in your load_image function. You dynamically allocate the array pointed to by skip. If any of the read operations between there and when you delete skip were to throw an exception you would leak that array. 256 bytes may also not be enough to read the entire color map. You should probably use a std::vector<char>. Better yet, rather than reading the values you don't want, you could just seek past them:

// Skip the ID String
file_stream.seekg(header.id_length, std::ios::cur);

// Skip the colour map if it doesn't exist
if (!(tgaDesc & 4)) {
    int colourMapSize = header.colour_map_type * header.num_entries;
    file_stream.seekg(colourMapSize, std::ios::cur);
}

Writing that example made me notice that tgaDesc is always 0, so that if block will always run. Did you mean to check header.colour_map_type here? Of course, if there's no color map then header.num_entries should be 0, so I'm not sure the if is even needed at all.

While we're in load_image, you pass the std::ios::ate flag when opening file_stream but then immediately seekg back to the beginning of the file. If you remove the std::ios::ate flag then the stream will initially be positioned at the beginning of the file and the extra seekg could be eliminated.

The way you read the file header is mostly fine. Byte ordering (AKA endianness) could be a possible issue, but both TGA and most modern CPUs use little-endian byte order so you're probably fine unless you want to support some esoteric platform.

Putting that all together would give you an image class that looks something like this:

class image {
public:
    TGA_HEADER header;
    std::vector<char> pixel_data;

    image(const std::string& image_path);
    ~image();

    void save_image(const std::string& file_name);
    void load_image(const std::string& path);
};

image::image(const std::string& image_path)
{
    load_image(image_path);
}

void image::load_image(const std::string& path)
{
    std::ifstream file_stream(path, std::ios::in | std::ios::binary);

    if (file_stream.is_open()) {
        int tgaDesc = 0;

        /* read header */
        file_stream.read(&header.id_length, sizeof(header.id_length));
        file_stream.read(&header.colour_map_type, sizeof(header.colour_map_type));
        file_stream.read(&header.image_type, sizeof(header.image_type));

        file_stream.read((char*)(&header.first_entry), sizeof(header.first_entry));
        file_stream.read((char*)(&header.num_entries), sizeof(header.num_entries));
        file_stream.read(&header.bits_per_entry, sizeof(header.bits_per_entry));

        file_stream.read((char*)(&header.x_origin), sizeof(header.x_origin));
        file_stream.read((char*)(&header.y_origin), sizeof(header.y_origin));
        file_stream.read((char*)(&header.width), sizeof(header.width));
        file_stream.read((char*)(&header.height), sizeof(header.height));
        file_stream.read(&header.bits_per_pixel, sizeof(header.bits_per_pixel));
        file_stream.read(&header.descriptor, sizeof(header.descriptor));

        // Skip the ID String
        file_stream.seekg(header.id_length, std::ios::cur);

        // Skip the colour map if it doesn't exist
        if (!(tgaDesc & 4)) {
            int colourMapSize = header.colour_map_type * header.num_entries;
            file_stream.seekg(colourMapSize, std::ios::cur);
        }

        int imageDataSize = header.width * header.height * (header.bits_per_pixel / 8);
        pixel_data.resize(imageDataSize);

        /* read image data */
        file_stream.read(pixel_data.data(), imageDataSize);
    }
}

Cannot pass std::unique_ptr in std::function

The std::function requires the function object to be Copy-Constructible, so you can't expect a lamdba to be moved to it. On initialization, it attempts to copy the lambda and so the std::unique_ptr with it, which is a member of this lambda, and, expectedly, fails to do so. What you can do is store your lambda in a variable and pass it to function that accepts const std::function& using std::ref like that:

        void foo(const std::function<void()>& f); // function declaration
        auto a = [h = std::move(handle)]() mutable
        {
            std::cout << *h << std::endl;
        };
        foo(std::ref(a));

This is a related question with much more detailed answers: How to create an std::function from a move-capturing lambda expression?

std::unique_ptr with custom deleter for win32 LocalFree

It looks correct to me. You could make it a little more succinct by specifying the unique_ptr's deleter inline rather than creating a functor for it.

std::unique_ptr<LPWSTR, HLOCAL(__stdcall *)(HLOCAL)> 
      p( ::CommandLineToArgvW( L"cmd.exe p1 p2 p3", &n ), ::LocalFree );

Or, if you don't want to mess with LocalFree's signature and calling conventions you can use a lambda to do the deletion.

std::unique_ptr<LPWSTR, void(*)(LPWSTR *)> 
      p( ::CommandLineToArgvW( L"cmd.exe p1 p2 p3", &n ), 
         [](LPWSTR *ptr){ ::LocalFree( ptr ); } );

Note: At the time this answer was first written, VS2010 was the released VS version available. It doesn't support conversion of capture-less lambdas to function pointers, so you'd have to use std::function in the second example

std::unique_ptr<LPWSTR, std::function<void(LPWSTR *)>> 
      p( ::CommandLineToArgvW( L"cmd.exe p1 p2 p3", &n ), 
         [](LPWSTR *ptr){ ::LocalFree( ptr ); } );

---

Related Topics