CODEWITHSUNDEEP
c++c++11
Kenneth
Kenneth

Reputation: 21

Properly return a unique ptr

I am writing a String class MyString (yes, as homework) and have to offer a toCString method which returns a unique_ptr<char[]> (and not a Vector). Unfortunately I fail when returning the pointer to the caller: The result is always filled with wrong content - it seems that I create the pointer and/or the character array on the stack.

unique_ptr<char[]> MyString::toCString() const {
     char *characters = new char[m_len];
     char *thisString = m_string.get();
     for (int i = 0; i < m_len; i++) {
         characters[i] = *(thisString + m_start + i);
     }
     const unique_ptr<char[], default_delete<char[]>> &cString = unique_ptr<new char[m_len]>(characters);
     return cString;
}

When debugging I always get expected behaviour. Problems only occur on callers site. Where is my mistake?

Upvotes: 1

Views: 3142

Answers (3)

Richard Hodges
Richard Hodges

Reputation: 69854

I see there is already an accepted answer, but this does not solve the problem. The problem on the client side is occurring because you're not null-terminating the c-string.

I don't know what type m_string is, so lets for a moment assume that it's a std::string. You can translate the actual methods yourself:

std::unique_ptr<char[]> MyString::toCString() const 
{
    // get length (in chars) of string
    auto nof_chars = m_string.size();

    // allocate that many chars +1 for the null terminator.
    auto cString = std::unique_ptr<char[]>{new char[nof_chars + 1]};

    // efficiently copy the data - compiler will replace memcpy
    // with an ultra-fast sequence of instructions in release build
    memcpy(cString.get(), m_string.data(), nof_chars * sizeof(char));

    // don't forget to null terminate!!
    cString[nof_chars] = '\0';

    // now allow RVO to return our unique_ptr
    return cString;
}

As per Christophe's suggestion, here's the method again, written in terms of std::copy_n. Note that the std::copy[_xxx] suite of functions all return an iterator that addresses one-past the last write. We can use that to save recomputing the location of the null terminator. Isn't the standard library wonderful?

std::unique_ptr<char[]> MyString::toCString() const 
{
    // get length (in chars) of string
    auto nof_chars = m_string.size();

    // allocate that many chars +1 for the null terminator.
    auto cString = std::unique_ptr<char[]>{new char[nof_chars + 1]};

    // efficiently copy the data - and don't forget to null terminate
    *std::copy_n(m_string.data(), nof_chars, cString.get()) = '\0';

    // now allow RVO to return our unique_ptr
    return cString;
}

Upvotes: 4

Gam
Gam

Reputation: 1272

Since you have edited your question, and now you are using 

unique_ptr<char[]> cString = unique_ptr<char[]>{new char[m_len]};

First improvement: use auto 

auto cString = unique_ptr<char[]>{new char[m_len]};

Second improvement: your tag is C+11, but if you happen to be using C+14, then use std::make_unique like this:

auto cString = std::make_unique<char[]>(m_len);

Further more, as Scott Meyers would say, if you are using C+11, then simply write the make_unique function yourself. It's not hard, and it's very very useful.

http://ideone.com/IIWyT0

template<class T, class... Types>
inline auto make_unique(Types&&... Args) -> typename std::enable_if<!std::is_array<T>::value, std::unique_ptr<T>>::type
{
    return (std::unique_ptr<T>(new T(std::forward<Types>(Args)...)));
}

template<class T>
inline auto make_unique(size_t Size) -> typename std::enable_if<std::is_array<T>::value && std::extent<T>::value == 0, std::unique_ptr<T>>::type
{
    return (std::unique_ptr<T>(new typename std::remove_extent<T>::type[Size]()));
}

Upvotes: 1

Christophe
Christophe

Reputation: 73366

Don't create a reference to a unique_ptr like you did. Instead, return the unique_ptr directly: the move constructor will take care of everything: 

 return unique_ptr<char[], default_delete<char[]>>(characters);

Upvotes: 2

Related Questions