String partial implementation using a vector

Question

I saw few similar solutions, but they all relied on C-style arrays and str utils, so I thought I would write a higher level C++ only implementation with a vector as an exercise. I have not implemented all the functions yet but wanted to first check if this looks to be on the right path or if there are some major issues.

class mystring {
public:
mystring() = default;
explicit mystring(const char* str) {
    if (str != nullptr) {

        data = std::vector<char>(str, str + strlen(str));
    }
}

mystring& operator=(const char* str) {
    data.clear();
    copy(str, str + strlen(str), back_inserter(data));
}

mystring& operator+=(const mystring& other) {
    data.insert(data.end(), other.data.begin(),other.data.end());
    return *this;
}

mystring operator+(const mystring& other) const {
    mystring result(other);
    result += other;
    return result;
}

explicit operator const char* () const {
    return c_str();
}

const char& operator[](size_t idx) const {
    return data[idx];
}

char& operator[](size_t idx) {
    return data[idx];
}

void replace(size_t b, size_t e, const char* str) {
    data.erase(data.begin()+b, data.begin() + e);
    data.insert(data.begin()+b,str,str+strlen(str));
}

friend ostream& operator<<(ostream& cout, const mystring& s);

bool operator==(const mystring& o) const =default;

const char* c_str() const {
    return data.data();
}

private:
std::vector<char> data;
};

Answer 1

Inconsistent handling of nullptrs

Your constructor checks if str might be nullptr, but your assignment operator does not. Choose whether you want to support nullptr arguments, and then handle it consistently.

I would personally choose to not allow nullptr arguments, and add assert(str) statements to check for that in debug builds. Consider that the C string functions also don't handle NULL.

Missing NUL-byte

When you construct a mystring, you only copy everything but the terminating NUL-byte. However, that means c_str() will return a pointer to a non-terminated string. Make sure you always have a terminating NUL-byte at the end of data.

Consider not adding any assignment operator

You declared an assignment operator that handles const char*s, but the compiler also generated a default assignment operator that takes another mystring as the right hand side, as if there was:

mystring& operator=(const mystring& other) = default;

If you would remove your custom assignment operator, then assignments from const char* will still work: since you have a constructor that takes const char*, the compiler will see that it can construct a new mystring from it first, and then assign it. Since it will also implement the move assignment operator automatically, it's even as efficient, or even more so (as copy(…, back_inserter(…)) might inefficiently insert one character at a time).

replace() is inefficient

Your replace() function will likely move parts of the string twice: once during erasure, and other time during insertion. It is probably more efficient to overwrite as much as possible, then just erase or insert the remainder.

std::string doesn't have an operator const char*()

There is no conversion operator from std::string to const char*. It's good that you made yours explicit, that avoids a lot of problems. However, it might be better to omit it altogether, to avoid any accidental conversions.

Answer 2

If you have unvalidated accesses operators:

const char& operator[](size_t idx) const {
    return data[idx];
}

char& operator[](size_t idx) {
    return data[idx];
}

Then you better have a way to validate that you are in range. So you really need to provide a size() or length() method!!!

---

Your c_str() is broken:

const char* c_str() const {
    return data.data();
}

If you are returning a C-String then it needs to be null-terminated. Your data is not null terminated (you don't copy the null from the C-String into your vector).