#include <string>
strcpy
and its friends reside within the C library header <string.h>
, which should be included as <cstring>
into a C++ program. Those functions then should be prepended with std::
, e.g. std::strcpy
. The C++ library versions defined in the C++ headers have overloads, which fixes some issues.
char* buff_;
You can replace that with std::unique_ptr<char[]>
. You won't have to implement the move functions nor the destructor when using a unique_ptr
data member.
Arguably, you should use a separate class for the whole memory management, following the Single Responsibility Principle. std::vector<char>
is a candidate, for example.
RawString(const char* str) {
std::cout << "rawstring normal char* ctor\n";
buff_ = new char[strlen(str) + 1];
strcpy(buff_, str);
buff_[strlen(str)] = '\0';
}
The C library string functions need to compute the length of the strings at each step. This imposes additional, unnecessary work. Additionally, the last step null-terminating buff_
should be unnecessary. Both strlen
and strcpy
already require that str
is null-terminated, and strcpy
copies the null-terminator.
RawString(const char* const str) {
std::cout << "rawstring normal char* ctor\n";
auto const src_len = std::strlen(str);
auto const src_len_with_nullterminator = len + 1;
buff_ = new char[src_len_with_nullterminator];
std::copy(str, str+src_len_with_nullterminator, buff_);
}
Also note that a user might already know the length of the string passed in; yet, there's no way to tell RawString
this length.
RawString(const RawString& str)
If you do not have an very memory-constrained system, consider adding a data member that stores the size of the string. You won't have to recompute it in this and other member functions.
RawString(RawString&& other)
Your move constructor and separate move assignment-operator (see below) as well as swap
should be marked as noexcept
. Not only exposes this a useful guarantee to the user via a language feature, but it is also important for other types that make their behaviour dependent on the noexcept-ness of those special member functions. The usual example is std::vector<RawString>
, which will provide some more efficient functions (push_back
etc.) when the value type has noexcept-move-functions.
RawString(char*&& other) {
std::cout << "rawstring move ctor\n";
buff_ = other;
other = nullptr;
}
For convenience, the converting constructor char const*
-> RawString
(similarly to std::string
) is typically implicit, even though it (potentially) is not cheap to construct a RawString
.
Your "moving" converting constructor is a constructor that takes ownership of a piece of memory. In my humble opinion, constructors that take ownership should be explicit. For example, it is rather simple to write this bug:
struct filesystem_entry
{
char name[FILENAME_MAX];
};
auto x = filesystem_entry{};
RawString s = x.name;
// operate on `s` for convenience
Similarly, implicit conversions appear e.g. for returning or passing arguments to a function (where the target type is not immediately visible). The issue is not solved entirely by making the constructor explicit, though:
RawString s( name_of_file() );
If one uses direct-initialization by default, this could be realistic code in my opinion.
Of course, one could argue that such behaviour is defined precisely by the interface of RawString
. I'd be surprised by such behaviour, though.
It is much safer to explicitly request a RawString
to take ownership of a raw pointer:
RawString(std::unique_ptr<char[]> str) noexcept
: buff_( str.release() )
{}
void swap(RawString& other) {
std::swap(other.buff_, buff_);
}
Nowadays, the guideline is: Implement swap
as a non-member friend function. This allows a generic usage of swap
as follows:
using std::swap;
swap(x, y);
This works for both built-in types and user-defined types alike.
Note that since you have an efficient move ctor and move assignment-operator, implementing swap
yourself is an optimization.
// RawString& operator=(const char* str)
With the OP's current implementation of operator=(RawString)
, implementing this assignment-operator is a more efficient than the implicit conversion used when it is not declared (const char*
-> RawString
implicit conversion, then using RawString::operator= (RawString)
). However, if you implement an operator=(RawString&&)
, the current implementation is no more efficient than the implicit conversion used without operator=(const char*)
.
It is possible though to implement an optimization for this as well as the RawString::operator=(RawString const&)
operator (see below), which makes this function useful.
RawString& operator=(RawString other)
Although this copy-and-swap technique is simple, it is not the most efficient(*): If the right hand side of the assignment is an lvalue, and the left hand side already has enough capacity to store the string of the RHS, you waste an allocation:
RawString myString("Hello World");
auto cpy = myString;
myString = cpy; // unnecessary allocation
It is therefore useful to define both separately:
RawString& operator=(RawString const&);
RawString& operator=(RawString&&);
If you follow my advice from above and move the memory management to another class, you can use the compiler-provided implementation for both in RawString
.
(*) Howard Hinnant often refers to this, e.g. in this StackOverflow answer. See also: Herb Sutter on "Don't overuse pass-by-value"
friend const RawString operator+(const RawString& lhs, const RawString& rhs)
Although it's almost a convention now, Alexander Stepanov (the inventor of the STL) opposes the usage of +
to concatenate strings. He argues that string concatenation is not commutative.
Anyway, in this implementation, we can see a possible inefficiency in the OP's allocation scheme. Classes like std::string
and std::vector
typically manage a block of memory that is only partially filled. This allows resizing by a certain factor once more memory is needed. An amortized constant complexity can be reached for growth this way.
Also, the OP's implementation uses C library string functions again.
Another inefficiency is that operator+
ignores the value category of its operands. Although an operator like +
is supposed to not modify its operands, it can do so if they're rvalues. This allows stealing the resources of any operand. For a similar reason, returning const
objects by values is detrimental. Consider:
RawString x("Hello");
auto s = x + " " + "World!";
We get a temporary string from x + " "
, but cannot use its resources for the concatenation of + "World!"
, even if its buffer had enough capacity. Furthermore, the initialization of s
requires yet another copy, since the returned RawString
is const
. s
will be a const RawString
, which prevents of course modification, even though that could be useful. Because of copy elision, no copy is made for the initialization of s
. Similarly, no copy is made for an assignment of the same form, if the operator=
takes a RawString
by value - on the other hand, declaring operator=(RawString&&)
and returning a non-const RawString
from operator+
should be no less efficient.
If you provide operator overloads, you should also make sure that there is some consistency. For example, operator+
corresponds to an operator+=
.
Speaking about operator overloads, an operator==
and operator<
(as well as its counterparts like >
) are generally very useful, e.g. for sorting.
(This leads to regular types.)
Oh, and by the way: There's currently no public interface to do something with the contents of the string, sadly. Depending on this interface, the default constructor might be dangerous: It does not initialize buff_
with the address of a null-terminated string (this is a nice class invariant).
Late addition: Oops, I missed a critical bug, related to that "nice class invariant" I mentioned above: This class invariant / guarantee is assumed to exist e.g. in the copy constructor:
RawString(const RawString& str) : RawString(str.buff_)
The converting constructor that is the target of the constructor delegation requires the argument to be a null-terminated string (because of the use of strlen
and friends).
Similarly, operator+
uses the same assumption.
However, this invariant is violated by the default constructor:
RawString()
{
std::cout << "rawstring def ctor\n";
buff_ = nullptr;
}
I can see two relatively simple solutions that maintain the class invariant:
RawString()
{
std::cout << "rawstring def ctor\n";
buff_ = new char[1]();
}
This makes the default constructor not noexcept (even though it's unlikely to throw), and it's probably not an efficient solution because of the allocation.
Solution two requires some more work in the destructor:
private:
static char* empty_string() {
static char str[] = {0};
return &str;
}
public:
RawString()
{
std::cout << "rawstring def ctor\n";
buff_ = empty_string();
}
~RawString()
{
if(empty_string() != buff_) delete[] buff_;
}
Another possibility is to give up on that class invariant, and allow buffer_ == nullptr
. As Loki Astari mentions in a comment to the OP, the bug can be fixed in that case by altering the code in RawString(char const*)
and operator+
.
null
being passed in as your string (don't want to do strlen(null). Also add a constructor that takesnullptr
.RawString(std::nullptr_t ptr) : RawString(static_cast<char*>(ptr)) {}
\$\endgroup\$ – Martin York Jan 6 '15 at 19:13