1

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?

jblixr
  • 1,345
  • 13
  • 34
Kenneth
  • 21
  • 2
  • 6

3 Answers3

4

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;
}
Richard Hodges
  • 68,278
  • 7
  • 90
  • 142
  • Apparently the object represents a substring of the `m_string`, hence `m_start` and `m_len`. But you are absolutely correct about making space and adding a NUL terminator. – Ben Voigt Apr 02 '16 at 15:48
  • Wouldn't it make sense to replace the use of the [old `memcpy()` with a `std::copy()` or `std::copy_n()`](http://stackoverflow.com/questions/4707012/is-it-better-to-use-stdmemcpy-or-stdcopy-in-terms-to-performance) ? – Christophe Apr 02 '16 at 15:53
  • @Christophe agreed, it would be more idiomatic. – Richard Hodges Apr 02 '16 at 15:54
  • @Christophe your suggestion added as a second version of toCString. I think your idea is better. – Richard Hodges Apr 02 '16 at 16:07
2

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);
Christophe
  • 68,716
  • 7
  • 72
  • 138
  • Also be certain that the c++ compiler supports and enables c++11. – Nathaniel Johnson Apr 02 '16 at 15:13
  • 2
    @NathanielJohnson sure ! On the other hand, if OP uses `unique_ptr` and not the deprecated `auto_ptr` and moreover could already compile some code with it, I can assume that C++11 is given ;-) – Christophe Apr 02 '16 at 15:21
  • 1
    what you meant to say was, "Return Value Optimisation will take care of everything". Returning efficiently by value doesn't even need a move constructor. – Richard Hodges Apr 02 '16 at 15:29
  • @RichardHodges yes, indeed: the copy/move elision rules for return values may make the situation even better ! However I didn't mention it in my answer, because the move construction is the real enabler, as according to 12.8/31, implementations are allowed to do copy/move elision but aren't obliged to do so. – Christophe Apr 02 '16 at 15:47
  • 2
    @Christophe understood. FYI There's a proposal before the committee to require copy elision in c++17. Here's hoping it passes! – Richard Hodges Apr 02 '16 at 16:00
  • @RichardHodges Interesting ! I wasn't aware, thanks. It would indeed be good thing. – Christophe Apr 02 '16 at 16:05
1

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]()));
}
Gam
  • 1,254
  • 1
  • 9
  • 18