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?

Improve this question
8
  • 1
    Why are you returning a smart pointer? C string is a mere pointer to char (and not to an array) Commented Apr 2, 2016 at 14:53
  • 1
    unique_ptr<new char[m_len]>(characters) This makes no sense, and doesn't compile. Show your actual code. Commented Apr 2, 2016 at 14:57
  • You are allocating the smart pointer on the stack as a reference, then exiting the function. This will produce unpredictable behavior even if that behavior seems to be correct. stackoverflow.com/questions/10643563/… Commented Apr 2, 2016 at 14:57
  • @NathanielJohnson The function returns by value - just as the article you cite recommends. It does not return a reference or a pointer to a local variable. The problem lies elsewhere. Commented Apr 2, 2016 at 14:58
  • Its a reference not a copy. Commented Apr 2, 2016 at 15:01

3 Answers 3

Reset to default
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;
}
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

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.
Wouldn't it make sense to replace the use of the old memcpy() with a std::copy() or std::copy_n() ?
@Christophe agreed, it would be more idiomatic.
@Christophe your suggestion added as a second version of toCString. I think your idea is better.
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);
Improve this answer

6 Comments

Also be certain that the c++ compiler supports and enables c++11.
@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 ;-)
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.
@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 understood. FYI There's a proposal before the committee to require copy elision in c++17. Here's hoping it passes!
|
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]()));
}
Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.