Implicit conversion of pointer to std::unique_ptr in foreach loop

Question

I have an api call which populates an array of raw pointers to be used by the caller. This function heap allocates each raw pointer but does not allocate the array.

I cannot change this API function regardless of how bad it is.

Calling the api function code looks something like this:

size_t response_count = api.getResponseCount();
std::vector<Response*> responses(response_count);
api.getResponses(responses.data());

for(auto response : responses) {
    // Do some processing with response
    delete response;
}

I would like to wrap each response in a unique_ptr such that it is still cleaned up at the end of the loop iteration without having to explicitly call delete. Ideally, this would look something like:

for(std::unique_ptr<Response> response : responses) {
    // Do some processing with response
    // No need to delete response, it will be cleaned up as it goes out of scope
}

This does not compile because the compiler cannot convert a pointer to a unique_ptr: error: conversion from ‘Response*’ to non-scalar type ‘std::unique_ptr<Response>’ requested

Is there way to cast each element of the container to a smart pointer in this way, or do I need to explicitly delete the raw pointer?

If it's a C function this is a terrible idea anyway. C doesn't `new` and consequently `delete` *anything*. So, again, what allocates the things coming back in that sequence of pointers? E.g. What does each pointer in the `responses` sequence point to, who exactly allocated whatever that is, and by what means was it allocated ? Just because you're using C++ doesn't mean some arbitrary dynamic pointer must have come from `new` and thus is ripe for `delete`. — WhozCraig, Nov 20 '19 at 23:21
Then `delete response` is invalid, right? Should be `free(response)`. — KamilCuk, Nov 20 '19 at 23:22
You could do something like a [custom deleter](https://stackoverflow.com/questions/19053351/how-do-i-use-a-custom-deleter-with-a-stdunique-ptr-member) if you wanted it to be RAII. — Kai, Nov 20 '19 at 23:22
@KamilCuk You are correct. It is actually C++ code now that I am looking at it. I will update the question to reflect this. I still cannot change the API as I am not in control of that code. — TheBat, Nov 20 '19 at 23:24
Even if it is C++ code, is it the same ABI (i.e. both modules, your exe/lib and the 'external' api lib are using the same shared dynamic runtime? If not, this is *still* a bad idea. If you have an external fixed API that allocates things, that API should have a complimentary mechanism for releasing them, or its design sucks. Is it a static lib? That may make it much, much cleaner. — WhozCraig, Nov 20 '19 at 23:26
@WhozCraig Yup, the design sucks. Legacy code from sometime in the '90s iirc. — TheBat, Nov 20 '19 at 23:28
So long as you're asking the questions I posed, and getting answers, you may be able to make this work. They're important. — WhozCraig, Nov 20 '19 at 23:29
@WhozCraig The top example I gave works and it is correct in managing the memory that this api allocates. I'm mostly looking for a cleaner way to write my code that wraps around it. This is code is directly linked (not an external library), the reasons I can't change it are not because I don't have access to the source, if that makes sense. — TheBat, Nov 20 '19 at 23:34
why don't you write yourself a minimal smart pointer that doesn't have an explicit constructor? — Thomas, Nov 20 '19 at 23:39
@Kai I don't see anything in the documentation for specifying a custom element deletor for a std::vector. In fact there is a note about vectors of pointers on the page for the vector destructor: https://en.cppreference.com/w/cpp/container/vector/~vector — TheBat, Nov 20 '19 at 23:41
Not a custom deletor for the vector, but for the unique_ptr in your loop. Custom deletors don't look nearly as clean, so the explicit delete in the loop (with a comment, perhaps?) might be the cleanest — Kai, Nov 20 '19 at 23:51
@Kai I can't even create the unique_ptr in the loop. The default destructor of the unique_ptr would be sufficient to delete the pointer. The problem is that foreach does not call the constructor for the unique pointer, but instead tries to assign the pointer into the unique_ptr container. Maybe if I had a `std::unique_ptr& operator=(Response*&& r);` it might work, but that is some really nasty boilerplate just for one instance. — TheBat, Nov 21 '19 at 00:00

score 3 · Answer 1 · answered Nov 20 '19 at 23:43

Instead of wrapping each pointer in a unique_ptr, I'd consider using a Boost ptr_vector.

Assuming the data is allocated so you actually can use delete to delete it, the code would look something like this:

size_t response_count = api.getResponseCount();

// Unfortunately, we have to define, then resize. It has a ctor that takes a size,
// but it treats that as an amount to reserve rather than an actual size.
boost::ptr_vector<Response> responses;
responses.resize(response_count);

api.getResponses(responses.c_array());

for(auto response : responses) {
    // Do some processing with response
}

...and when responses goes out of scope, it will delete all the objects pointed to by the pointers it contains. If necessary, you can specify an Allocator class that defines how the objects are allocated and deleted.

Reference

https://www.boost.org/doc/libs/1_71_0/libs/ptr_container/doc/ptr_container.html

score 2 · Accepted Answer · edited Nov 21 '19 at 18:05

2

Though not considered good practice, you can derive from unique_ptr with a non explicit constructor.

template<typename P>
struct MakeUnique : std::unique_ptr<P> {
    MakeUnique(P* p) : std::unique_ptr<P>(p) {}
};

This can then be used like this:

for ( MakeUnique<Response> resp : responses ) {
    ...
}

Probably the closest thing to a one-liner. See working version here.

edited Nov 21 '19 at 18:05

TheBat

1,006
7
25

answered Nov 21 '19 at 07:53

Thomas

4,980
2
15
30

I've proposed an edit to the code to make it use templates and be a bit more generic. I like this solution since it's minimum boilerplate and doesn't rely on boost. – TheBat Nov 21 '19 at 17:55

Thomas · Answer 3 · 2019-11-21T07:54:14.023

You can create a wrapper for the vector<response*> that hands out unique pointers

struct wrapper{

    struct iterator {
        iterator( std::vector<response*>::iterator it ) : it_(it){}
        friend bool operator!=( iterator const& lhs, iterator const &rhs ){ return lhs.it_ != rhs.it_; }
        void operator++(){ ++it_;}
        std::unique_ptr<response> operator*(){ return std::unique_ptr<response>(*it_); }
    private:   
        std::vector<response*>::iterator it_;
    };

    wrapper( std::vector<response*>& rs ) : rs_{rs} {}

    iterator begin() const { return iterator{rs_.begin()}; }
    iterator end() const { return iterator{rs_.end()}; }

private:
    std::vector<response*>& rs_;
};

You can then iterate over the responses like this:

for( auto resp : wrapper( responses ) ){...
}

See working version here.

Implicit conversion of pointer to std::unique_ptr in foreach loop

3 Answers3