C++ string operators

Question

I'm trying to practice my C++ on my own; At the moment I'm trying to implement the concept of operator overloading making my own string class/function.

I think I handled this correctly; the code runs successfully. However I'm not so sure if my code is optimally written (do I need to make more methods/params const? Do I use to much excess code?) and did I do good memory management?

Thanks for all the feedback :)

#include <iostream>
#include <vector>
#include <string.h>
#include <stdio.h>
using namespace std;
class DynString {
private:
 char *c;
public:
 ~DynString() { delete[]c; }
 DynString() : c(NULL) {}
 //second constructor
 DynString(const char* input) {
 int x = strlen(input) + 1;
 c = new char[x];
 strcpy(c, input);
 }
 //(copyconstructor) TRIO //QUATRO
 DynString(const DynString& dynstring) {
 c = new char[strlen(dynstring.c) + 1];
 strcpy(this->c, dynstring.c);
 }
 //UNO
 friend ostream& operator<< (ostream& lhs, DynString& a) {
 lhs << a.c;
 return lhs;
 }
 //DUO
 char& operator[] (int i) {
 return c[i];
 }
 //TRIO
 DynString& operator+ (DynString& y) {
 char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
 strcpy(newArray, this->c);
 strcat(newArray, y.c);
 delete[] c;
 c = new char[strlen(newArray)];
 this->c = newArray; 
 return *this;
 }
 //QUATRO
 friend DynString operator+ (char* x, DynString& y) {
 DynString newString;
 newString.c = new char[strlen(x) + strlen(y.c) + 1];
 strcpy(newString.c, x);
 strcat(newString.c, y.c);
 return newString;
 }
 //QUATRO
 DynString& operator+ (char* x) {
 char *newArray = new char[strlen(x) + strlen(this->c) + 1];
 strcpy(newArray, this->c);
 strcat(newArray, x);
 delete[] c; 
 c = new char[strlen(newArray)];
 this->c = newArray;
 return *this;
 }
 //TRIO // QUATRO
 DynString& operator= (DynString& x) {
 char *newArray = new char[strlen(x.c) + 1];
 strcpy(newArray, x.c);
 delete[] c;
 c = new char[strlen(newArray)];
 this->c = newArray;
 return *this;
 }
};
int main()
{
 DynString stringA("Hello");
 DynString stringB(" Worlt");
 //UNO
 cout << stringA << stringB << std::endl;
 //DUO
 stringB[5] = 'd';
 //TRIO
 DynString stringC = stringA + stringB;
 cout << stringC << std::endl;
 DynString stringD;
 //QUATRO
 stringD = "The" + stringB + " Wide Web";
 cout << stringD << std::endl;
 system("pause");
 return 0;
}

• jvb: More methods are needed with regard to const-ness :) Once you start with const DynString (and I hope you will start with const), you will discover operator[] won't work. You need two operators, one for read-only access and one for modifying. Same with operator= - at the moment, you can't use a const object on the right hand side!
• Toby Speight: Just to avoid confusion, you don't need two versions of operator= - but you do need to make its argument const. Whereas operator[] does need two versions: one returning const char& and the other returning char&.
• doug65536: @jvb Why do you hope he will start with const? const applies to the thing before the const. char * const x is read right to left "x is a constant pointer to char". Another example: char const * x reads "x is a pointer to a constant char". Your advice to write it in the reverse order only prevents understanding more complex cases.
• jvb: @doug65536 I expressed my hope the author of the source would start using const, as there was not a single instance of this keyword to find. - Please, now, don't interpret this as advice to use using.
• doug65536: @jvb Ah, sorry I misunderstood.

Answer 1

Don't include unnecessary headers

We're not using <vector> or <stdio.h>, so drop them. We only need <ostream> rather than <iostream> for the class, and we prefer <cstring> to <string.h> to place the standard functions in their proper namespace.

Avoid importing all of namespace std

It's a bad habit that will cause problems in bigger programs. Just don't.

Add some compiler warnings

I get some useful diagnostics, worth heeding, with g++ -Weffc++:

165851.cpp:33:39: warning: ‘DynString& DynString::operator+(DynString&)’ should return by value [-Weffc++]
 DynString& operator+ (DynString& y) {
 ^
165851.cpp:54:34: warning: ‘DynString& DynString::operator+(char*)’ should return by value [-Weffc++]
 DynString& operator+ (char* x) {
 ^
165851.cpp: In constructor ‘DynString::DynString(const char*)’:
165851.cpp:12:5: warning: ‘DynString::c’ should be initialized in the member initialization list [-Weffc++]
 DynString(const char* input) {
 ^~~~~~~~~
165851.cpp: In copy constructor ‘DynString::DynString(const DynString&)’:
165851.cpp:19:5: warning: ‘DynString::c’ should be initialized in the member initialization list [-Weffc++]
 DynString(const DynString& dynstring) {
 ^~~~~~~~~
165851.cpp: In function ‘int main()’:
165851.cpp:95:23: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wpedantic]
 stringD = "The" + stringB + " Wide Web";
 ^~~~~~~
165851.cpp:95:33: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wpedantic]
 stringD = "The" + stringB + " Wide Web";
 ^~~~~~~~~~~

The last two are easily fixed by changing operator+ to accept const char* instead of char*.

The usual way of fixing the warnings on operator+ is to implement operator+ in terms of operator+= (the latter returns a reference, but the former an object).

BUG - be careful using a null pointer as sentinel

What happens if you evaluate DynString() + DynString()? In both cases, the internal pointer points to NULL, but you happily call strlen on both - that's Undefined Behaviour. Here's some code that actually checks:

DynString(const DynString& other)
 : c(nullptr)
{
 if (other.c) {
 c = new char[std::strlen(other.c) + 1];
 std::strcpy(c, other.c);
 }
}

But it's probably easier (safer) to always allocate memory:

DynString(const char* input)
 : c(new char[(input ? std::strlen(input) : 0) + 1];)
{
 if (input) {
 std::strcpy(c, input);
 } else {
 *c = '0円';
 }
}

And then all methods can safely assume c isn't null.

Add a move constructor

This one's easy:

DynString(DynString&& other)
 : DynString()
{
 std::swap(c, other.c);
}

Overload operator[]

Allow for both constant and mutable string objects:

char& operator[](size_t i) {
 return c[i];
}
const char& operator[](size_t i) const {
 return c[i];
}

I believe size_t is more appropriate than int as the type of the index.

Eliminate the memory leaks

The example program leaks:

==13517== Memcheck, a memory error detector
==13517== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==13517== Using Valgrind-3.12.0.SVN and LibVEX; rerun with -h for copyright info
==13517== Command: ./165851
==13517== 
Hello Worlt
Hello World
The World Wide Web
sh: 1: pause: not found
==13517== 
==13517== HEAP SUMMARY:
==13517== in use at exit: 47 bytes in 3 blocks
==13517== total heap usage: 12 allocs, 9 frees, 73,860 bytes allocated
==13517=わ=わ 
=わ=わ13517=わ=わ 11 bytes in 1 blocks are definitely lost in loss record 1 of 3
==13517== at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517== by 0x108FF3: DynString::operator+(DynString&) (165851.cpp:38)
==13517== by 0x108C62: main (165851.cpp:90)
==13517=わ=わ 
=わ=わ13517=わ=わ 18 bytes in 1 blocks are definitely lost in loss record 2 of 3
==13517== at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517== by 0x10915B: DynString::operator+(char*) (165851.cpp:59)
==13517== by 0x108CD5: main (165851.cpp:95)
==13517=わ=わ 
=わ=わ13517=わ=わ 18 bytes in 1 blocks are definitely lost in loss record 3 of 3
==13517== at 0x4C2C93F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13517== by 0x1091EF: DynString::operator=(DynString&) (165851.cpp:69)
==13517== by 0x108CE7: main (165851.cpp:95)
==13517== 
==13517== LEAK SUMMARY:
==13517== definitely lost: 47 bytes in 3 blocks
==13517== indirectly lost: 0 bytes in 0 blocks
==13517== possibly lost: 0 bytes in 0 blocks
==13517== still reachable: 0 bytes in 0 blocks
==13517== suppressed: 0 bytes in 0 blocks
==13517== 
==13517== For counts of detected and suppressed errors, rerun with: -v
==13517== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 0 from 0)

The main cause is:

 char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
 strcpy(newArray, this->c);
 strcat(newArray, y.c);
 delete[] c;
 c = new char[strlen(newArray)];
 this->c = newArray;

The error is more visible if you remove the redundant this-> from the last line:

 delete[] c;
 c = new char[strlen(newArray)];
 c = newArray;

The first allocation is obviously leaked, and that line of code should be deleted. The same error is present in several other methods.

Implementation of operator+:

I would write it like this, starting with the += that the other versions need:

public:
 DynString& operator+=(const DynString& other)
 {
 char *newArray = new char[strlen(c) + strlen(other.c) + 1];
 std::strcpy(newArray, c);
 std::strcat(newArray, other.c);
 delete[] c;
 c = newArray;
 return *this;
 }
 DynString operator+(const DynString& other) const
 {
 DynString newString(*this);
 return newString += other;
 }

We don't need operator+(const char*), as a C-style string will be promoted to DynString using the (non-explicit) constructor. We do need an overload to take a C-style string as the first argument - but it doesn't need to be a friend, as it can be an ordinary non-member function instead:

DynString operator+(const char* x, const DynString& other) {
 DynString newString(x);
 return newString += other;
}

---

Improved version

This code is tested and does not leak:

#include <ostream>
#include <cstring>
class DynString {
private:
 char *c;
public:
 ~DynString()
 {
 delete[] c;
 }
 DynString(const char* input = nullptr)
 : c(new char[(input ? std::strlen(input) : 0) + 1])
 {
 if (input) {
 std::strcpy(c, input);
 } else {
 *c = '0円';
 }
 }
 DynString(const DynString& other)
 : c(new char[std::strlen(other.c) + 1])
 {
 std::strcpy(c, other.c);
 }
 DynString(DynString&& other)
 : DynString()
 {
 std::swap(c, other.c);
 }
 friend std::ostream& operator<<(std::ostream& lhs, const DynString& a)
 {
 return lhs << a.c;
 }
 char& operator[](size_t i)
 {
 return c[i];
 }
 const char& operator[](size_t i) const
 {
 return c[i];
 }
 DynString& operator+=(const DynString& other)
 {
 char *newArray = new char[strlen(c) + strlen(other.c) + 1];
 std::strcpy(newArray, c);
 std::strcat(newArray, other.c);
 delete[] c;
 c = newArray;
 return *this;
 }
 DynString operator+(const DynString& other) const
 {
 DynString newString(*this);
 return newString += other;
 }
 DynString& operator=(DynString other) {
 std::swap(c, other.c);
 return *this;
 }
};
DynString operator+(const char* x, DynString& other) {
 DynString newString(x);
 return newString += other;
}
#include <iostream>
int main()
{
 DynString stringA("Hello");
 DynString stringB(" Worlt");
 //UNO
 std::cout << stringA << stringB << std::endl;
 //DUO
 stringB[5] = 'd';
 //TRIO
 DynString stringC = stringA + stringB;
 std::cout << stringC << std::endl;
 DynString stringD;
 //QUATRO
 stringD = "The" + stringB + " Wide Web";
 std::cout << stringD << std::endl;
}

---

Make the concatenation more efficient

Implementing operator+ by copying and appending is inefficient because it allocates memory for the copy that is immediately released during the append. So we can revisit that method and eliminate the copy step. I'd do it using an extra constructor:

private:
 DynString(const DynString& a, const DynString& b)
 : c(new char[std::strlen(a.c) + std::strlen(b.c) + 1])
 {
 std::strcpy(c, a.c);
 std::strcat(c, b.c);
 }
public:
 DynString operator+(const DynString& other) const
 {
 return DynString(*this, other);
 }

We can now simplify operator+=, implementing that in terms of our new operator+:

void swap(DynString& other)
{
 std::swap(length, other.length);
 std::swap(c, other.c);
}
DynString& operator+=(const DynString& other)
{
 Dynstring newString(*this, other);
 swap(newString);
 return *this;
}

Remember the string length

We can make every method more efficient, and also allow for embedded NUL characters in strings, if we store the length as a member:

private:
 std::size_t length; // not including terminator
 char *c;
 DynString(const DynString& a, const DynString& b)
 : length(a.length + b.length),
 c(new char[length + 1])
 {
 std::memcpy(c, a.c, a.length);
 std::memcpy(c+a.length, b.c, b.length+1); // copies terminator
 }

Lean on the standard library for memory management

Instead of managing our own memory, we could make c be a unique pointer:

private:
 std::size_t length; // not including terminator
 std::unique_ptr<char[]> c;
 DynString(const DynString& a, const DynString& b)
 : length(a.length + b.length),
 c(std::make_unique<char[]>(length + 1))
 {
 std::memcpy(c.get(), a.c.get(), a.length);
 std::memcpy(c.get()+a.length, b.c.get(), b.length+1); // copies terminator
 }

You'll see that we do need to sprinkle calls to get() where we need pointers to the contents

---

Improved, efficient, smart-pointer version:

(just the class; everything else unchanged from first "Improved version" above, except an additional include)

#include <memory>
class DynString {
private:
 std::size_t length; // not including terminator
 std::unique_ptr<char[]> c;
 // Helper constructor - allocates but does not initialize
 DynString(size_t length)
 : length(length),
 c(std::make_unique<char[]>(length + 1))
 {}
 DynString(const DynString& a, const DynString& b)
 : DynString(a.length + b.length)
 {
 std::memcpy(c.get(), a.c.get(), a.length);
 std::memcpy(c.get()+a.length, b.c.get(), b.length+1); // copies terminator
 }
public:
 DynString(const char* input = nullptr)
 : DynString(input ? std::strlen(input) : 0)
 {
 if (input) {
 std::memcpy(c.get(), input, length + 1);
 } else {
 c.get()[0] = '0円';
 }
 }
 DynString(const DynString& other)
 : DynString(other.length)
 {
 if (length != other.length)
 c = std::make_unique<char[]>(other.length+1);
 length = other.length;
 std::memcpy(c.get(), other.c.get(), length + 1);
 }
 DynString(DynString&& other) noexcept
 : DynString()
 {
 swap(other);
 }
 void swap(DynString& other) noexcept
 {
 std::swap(length, other.length);
 std::swap(c, other.c);
 }
 friend std::ostream& operator<<(std::ostream& lhs, const DynString& a)
 {
 return lhs << a.c.get();
 }
 char& operator[](size_t i) noexcept
 {
 return c.get()[i];
 }
 const char& operator[](size_t i) const noexcept
 {
 return c.get()[i];
 }
 DynString& operator+=(const DynString& other)
 {
 DynString newString(*this, other);
 swap(newString);
 return *this;
 }
 DynString operator+(const DynString& other) const
 {
 return DynString(*this, other);
 }
 DynString& operator=(DynString other) noexcept
 {
 swap(other);
 return *this;
 }
};

• Deduplicator: Let me criticize your review: 1. The c*-headers don't guarantee to confine their symbols to namespace std. They only guarantee to also put them there. 2. Implementing operator+ in terms of operator+= is unconscionably inefficient for this class.
• Toby Speight: @Deduplicator, I didn't mean to imply that the headers mustn't declare anything outside namespace std. I do find it makes my code clearer if I can be explicit about where functions come from. I think I probably also assume that it reduces risk of accidentally using a reserved identifier, but that's probably illusory! As to the efficiency, you're certainly right there, but I wanted to focus on correctness before optimising further (and real-world implementations go much further, e.g. SSO).
• Deduplicator: That's certainly true enough. But those further optimizations all somewhat depend on the string being counted...
• Pysis: I just still find this point convincing when it is apparently too fashionable to criticize using namespace std. stackoverflow.com/questions/1452721/… Why overload standard library function names from a third-party library?
• Rakete1111: @Pysis A simple variable data or count is enough to make the compiler complain.

Answer 2

1. Don't include what you don't use. You only use <string.h> and <stdio.h>.
   While including <vector> only costs compile-time, including <iostream> potentially increases your executable-size and startup-/shutdown-time.

2. You should avoid using namespace std; It's a plague and just biding its time to bite you later.
   Why is "using namespace std;" considered bad practice?

3. I'm sorry, but your comments seem to be completely useless. At least I couldn't extract any useful information from them. So, purge them.

4. Your DynString seems to be a thin wrapper around 0-terminated strings. Are you sure you will never need embedded zeroes, nor have enough use of the length that caching it might be worthwhile?
   Another disadvantage is that due to the also missing capacity appending to the string will always re-allocate.

5. Raw owning pointers are a bad idea. Not only are they leak-prone per se, things get even harder with exceptions.
   Consider a move to the no-cost abstraction of std::unique_ptr.

6. Your nullptr-handling is inconsistent. Is nullptr UB, an empty string, or preserved as a separate state?

7. As an aside, prefer nullptr to NULL or 0. Not only does it have a distinctive type, it also cannot be confused with an integer.

8. Use std::size_t for indices and sizes. That's what it's for.

9. You forgot to mark your functions noexcept where allowed.

10. Your default-constructor should be marked constexpr, or allocate a 0-length string.

11. In DynString(const char*) you calculate the length of the input for allocating the proper amount. Why don't you reuse that for an efficient memcpy instead of doing a less performant strcpy?
    At least it makes sense to allow using this for implicit conversions. Otherwise, you would have to mark it explicit.

12. DynString(const DynString&) should delegate to DynString(const char*) instead of writing everything out all over again.

13. You are missing a move-ctor.

14. The stream-output-operator does not modify the second argument, and should thuss accept it by constant reference.

15. You are missing a constant overload for operator []. And anyway, the index should be a std::size_t, not an int.

16. Your operator+(DynString&) (and operator+(const char*) too) is a first-grade mess:

    • It should return a new DynString by value, not one of the arguments by reference.
    • Both arguments should be constant.
    • You shouldn't allocate a second time in there just to leak it. Besides, that second allocation could also throw...
    • It should be a friend-function, thus enabling conversion on its first argument, delegating to a private method concatenating two const char*s. Doing so allows you to implement the two variants replacing one of the arguments with a const char* for efficiency without duplication.
    • Why do you not reuse the string-lengths you calculated? memcpy is more efficient than strcpy.

17. At least operator+ (char* x, DynString& y) is not wrong, only inefficient.

18. Consider adding operator+= (both varieties for efficiency), implemented in terms of operator+ and operator=.

19. Your operator= should not accept an lvalue-reference. Accept the argument by value and use copy-and-swap. That way you also lose thememory-leak.

20. You really should add some more observers, to wit: .data(), .size(), .begin() and .end() at least.

Modified code, deciding that nullptr is UB, and throwing in some extras:

#include <cstring>
#include <algorithm>
#include <utility>
#include <iterator>
#include <vector>
class DynString {
 char *c;
 DynString(std::picewise_construct_t, const char* a, const char* b) : c() {
 auto na = std::strlen(a), nb = std::strlen(b);
 c = new char[a + b + 1 > a ? a + b + 1 : SIZE_MAX];
 std::copy_n(a, na, c);
 std::copy_n(b, nb + 1, c + na);
 }
 template<class It, decltype((void)(*c = *std::declval<It>()))...>
 DynString(std::forward_iterator_tag, It first, It last) : c() {
 auto n = std::distance(first, last);
 c = new char[0 <= n && n < SIZE_MAX ? std::size_t(n) + 1 : SIZE_MAX];
 std::copy_n(first, std::size_t(n), c);
 c[n] = 0;
 }
 template<class It, decltype((void)(*c = *std::declval<It>()))...>
 DynString(std::input_iterator_tag, It first, It last) : c() {
 std::vector<char> v(first, last);
 swap({v.begin(), v.end()});
 }
public:
 DynString() : c(new char[1]{}) {}
 DynString(const char* s) : DynString(s, s + std::strlen(s)) {}
 DynString(const DynString& s) : DynString(s.c) {}
 DynString(DynString&& s) : DynString() { swap(s); }
 template<class It, decltype((void)DynString(std::iterator_traits<It>::iterator_category(), first, last))...>
 DynString(It first, It last) : DynString(std::iterator_traits<It>::iterator_category(), first, last) {}
 ~DynString() { delete[] c; }
 friend ostream& operator<<(ostream& lhs, const DynString& a) { return lhs << a.c; }
 char& operator[](std::size_t i) noexcept { return c[i]; }
 const char& operator[](std::size_t i) const noexcept { return c[i]; }
 auto data() noexcept { return c; }
 auto data() const noexcept { return c; }
 auto begin() noexcept { return c; }
 auto begin() const noexcept { return c; }
 auto end() noexcept { return c + size(); }
 auto end() const noexcept { return c + size(); }
 auto rbegin() noexcept { return std::make_reverse_iterator(end()); }
 auto rbegin() const noexcept { return std::make_reverse_iterator(end()); }
 auto rend() noexcept { return std::make_reverse_iterator(begin()); }
 auto rend() const noexcept { return std::make_reverse_iterator(begin()); }
 auto size() const noexcept { return std::strlen(c); }
 friend DynString operator+(const DynString& a, const DynString& b)
 { return {std::piecewise_construct, a.c, b.c}; }
 friend DynString operator+(const char* a, const DynString& b)
 { return {std::piecewise_construct, a, b.c}; }
 friend DynString operator+(const DynString& a, const char* b)
 { return {std::piecewise_construct, a.c, b}; }
 DynString& operator+=(const DynString& s) { return *this += s.c; }
 DynString& operator+=(const char* s) { return *this = *this + s; }
 DynString& operator=(DynString s) { swap(s); return *this; }
 void swap(DynString& o) noexcept { using std::swap; swap(c, o.c); }
};

• Toby Speight: I meant to say something about embedded '0円' in my answer, but I forgot. Now I don't need to ;-)

Answer 3

Memory leak here (in operator+):

 c = new char[strlen(newArray)];
 this->c = newArray;

because there is no difference between c and this->c. The code will overwrite the pointer to the new-allocated memory.

Answer 4

One thing I think the other answers missed: you're using strcat, strcpy and strlen. It's 2017, you're writing C++ -- never do this.

enter image description here

Instead of writing C++ as if you're writing C in 1978, please use the strn* family of functions: strncat, strncpy, and a strnlen implementation: Microsoft's strnlen, or the GNU strnlen if you are using glibc (Linux, some BSDs, etc).

Additionally, in C++ you have access to nice functions like std::copy and std::find.

Doing so makes your code inherently safer (less prone to buffer overflows), more predictable and reliable, and far more modern.

• David Conrad: Note: You don't have to use the bad old versions of those functions in C anymore, either.
• Konrad Rudolph: Why not go the extra mile, purge the str* functions completely, and use proper C++ algorithms? I.e. std::copy (for copying and concatenating), std::find (for length). Admittedly, I’d use std::strnlen if it existed; but it doesn’t. I guess std::strlen is OK if you’re operating on a buffer of unknown size (but then, this shouldn’t happen).
• cat: @DavidConrad I'm a C programmer first and a C++ programmer not at all, so that's how I know about the cases for strn* :)
• cat: @Deduplicator I'm sorry, what? Did you just say there are times it's okay to use the unsafe and deprecated versions of these functions?
• Deduplicator: @cat: First, only MS deprecated them, and it's a pain. Second, use the right tool for the job, and that's virtually never those functions. Did you ever take a close look at their API-definitions, especially strncpy and strncat? Anyway, str*cat suffers from a severe case of Shlemiel The Painter! (Using strcpy only exhibits weak symptoms, you should use std::copy_n or such when you have the length.)

Answer 5

operator+ implementation has a couple MAJOR bugs

DynString& operator+ (DynString& y) {
 char *newArray = new char[strlen(y.c) + strlen(this->c) + 1];
 strcpy(newArray, this->c);
 strcat(newArray, y.c);
 delete[] c;
 c = new char[strlen(newArray)];
 this->c = newArray; 
 return *this;
}

This implementation will cause some pretty bad side effects. Consider the following cases where A,B and C are DynString types in the current scope:

DynString A = "foo";
DynString B = "bar";
A = A + B; //this example works and does what it looks like it should

Now Consider:

DynString A = "foo";
DynString B = "bar";
DynString C = A + B; //this example not work like it should

or alternatively

DynString A = "foo";
DynString B = "bar";
A + B; //this example not work like it should

These examples have unintended side effects. After running this block the strings store the following values:

B == "bar" (Expected Result)
C == "foobar" (Expected Result, in case where C exist)
A == "foobar" (Unexpected Result!)

Lastly, we're returning by reference so consider this case:

DynString A = "foo";
DynString B = A + "bar" + "dude"; //contrived example, but clear point

After running the above case we expect that B is "foobardude" and what we expect of A is dependent on whether we're aware of the bug in the previous example. We should expect A is either "foo" or "foobar" because a temp should be returned. Neither of those will be the case, A will be "foobardude". As written, that implementation won't allow concatenation without altering a variable's original value.

Answer 6

Your destructor will try to free the memory even if it has not been allocated. Maybe create a boolean condition to check for that.

• Deduplicator: That's not a defect. Remember that deleting a null-pointer is a no-op.