1
\$\begingroup\$

Any input is appreciated! And can I get some advice on how to properly delete the dynamically allocated memory with the destructor? My professor said we have to delete each individual element of an array before doing delete [] array, and I'm not sure how I would do that here because I don't know the length of the array.

These are the instructions for the assignment http://voyager.deanza.edu/~bentley/cis22b/ass8.html

#include <iostream>
#include <cstring>


using namespace std;


class String
{
private:
    char* data;
    static int numA;

public:
    String();
    String(const string&);
    String(const String&);
    ~String();
    static int a_count()
    {
        return numA;
    }
    void countA();
    void lowerNumA();
    bool operator < (const String& obj) const;
    bool operator > (const String& obj) const;
    bool operator == (const String& obj) const;
    void operator = (const String& obj);
    void operator += (const String& obj);
    char* operator + (const String& obj) const;
    int operator ! () const;
    char operator[] (int) const;
    char* operator * () const
    {
        return data;
    }
    friend ostream & operator << (ostream&, String&);

};

String::String()
    {
        data = new char[0];
        data[0] = '\0';
        countA();
    }


String::String(const string& thing)
    {
        data = new char[0];
        strcpy(data, thing.c_str());
        countA();
    }

String::String(const String &s2)
    {
        data = s2.data;
        countA();
    }

String::~String()
{
    delete [] data;
}

bool String::operator < (String const &obj) const
    {
        if(strcmp(data, obj.data) < 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    }

void String::countA()
    {
        for(int i = 0; data[i] != '\0'; i++)
        {
            if(data[i] == 'a' || data[i] == 'A')
                {
                    numA++;
                }
        }
    }


void String::lowerNumA()
    {
        for(int i = 0; data[i] != '\0'; i++)
        {
            if(data[i]=='a' || data[i]=='A')
            {
                numA--;
            }
        }
    }


bool String::operator > (String const &obj) const
    {
        if(strcmp(data, obj.data) > 0)
        {
            return true;
        }
        else
        {
            return false;
        }
    }

bool String::operator == (String const &obj) const
    {

        if(strcmp(data, obj.data) == 0)
        {
            return true;
        }
        else
        {
            return false;
        }

    }

void String::operator = (String const &obj)
    {
        lowerNumA();
        data = obj.data;

        countA();
    }

void String::operator += (String const &obj)
    {
        lowerNumA();
        int element = 0;

        while(data[element] != '\0')
        {
            element++;
        }

        int element2 = 0;

        while(obj.data[element2] != '\0')
        {
            data[element] = obj.data[element2];
            element++;
            element2++;
        }

        data[element]='\0';
        countA();
    }

char* String::operator + (String const &obj) const
    {
        char* newChar = new char[100];
        int newCount = 0;
        int index = 0;

        while(data[index]!='\0')
        {
            newChar[newCount]= data[index];
            newCount++;
            index++;
        }

        index = 0;

        while(obj.data[index] != '\0')
        {
            newChar[newCount] = obj.data[index];
            newCount++;
            index++;
        }

        newChar[newCount] = '\0';

        cout << "";

        return newChar;

    }

int String::operator ! () const
    {
        int index = 0;
        int size = 0;

        while(data[index] != '\0')
        {
            size++;
            index++;
        }

        return size;
    }

char String::operator[] (int inIndex) const
    {
        int index = 0;
        while(data[index] != '\0')
        {
            index++;
        }

        if(inIndex >= 0 && inIndex <= index)
        {
            return data[inIndex];
        }
        else
        {
            cout<<"out of bound\n";
            return ' ';
        }
    }


int String::numA;

int main()
{
    // Constructors
    String A("apple");
    String B("banana");
    String C("cantaloupe");
    String D(B);
    String E;

    // static member function
    cout << "Number of a's = " << String::a_count() << endl << endl;

    // Overloaded insertion operator
    cout << "A = " << A << endl;
    cout << "B = " << B << endl;
    cout << "C = " << C << endl;
    cout << "D = " << D << endl;
    cout << "E = " << E << endl << endl;

    // Relational operators
    cout << boolalpha;
    cout << "A < B " << (A < B) << endl;
    cout << "B < A " << (B < A) << endl;
    cout << "A == B " << (A == B) << endl << endl;

    // Assignment operator
    A = B;
    cout << "A = " << A << endl;
    cout << "A == B " << (A == B) << endl << endl;

    // Size (bang) operator
    cout << "A size = " << !A << endl;
    cout << "E size = " << !E << endl << endl;

    // Unary * operator
    cout << "C text = " << *C << endl << endl;

    // Plus operator
    cout << "A + B = " << A + B << endl << endl;

    // Plus equal operator
    A += C;
    cout << "A = " << A << endl << endl;

    // Index operator
    cout << "A[3] = " << A[3] << endl << endl;

    // static member function
    cout << "Number of a's = " << String::a_count() << endl;
}

ostream & operator << (ostream& out, String& things)
{
    int index = 0;

    while(things.data[index] != '\0')
    {
        out << things.data[index];
        index++;
    }

    return out;
}
\$\endgroup\$
  • \$\begingroup\$ professor said we have to delete each individual element of an array before doing delete [] array another cases where the written form had ("would have") distinct advantages, starting with std::array vs. [] - neither features in the text linked. StackOverflow is not AssignmentUnload: can you turn your post into an answerable programming question? (Heed item 3: Questions asking for homework help must include a summary of the work you've done so far….) \$\endgroup\$ – greybeard Mar 13 '18 at 7:19
  • \$\begingroup\$ (String::a_count() makes me remark users of AE don't consistently spell arse.) \$\endgroup\$ – greybeard Mar 13 '18 at 7:30
3
\$\begingroup\$

Good things

In case this review sounds harsh, I'll start with a couple of things I really like about this code:

  • You've taken care matching new[] with delete[] - that's easy to mess up
  • You're taking care with const (you did miss one, though - try changing B to const String and you'll spot it).

Avoid using namespace std;

Importing all names of a namespace is a bad habit to get into, and can cause surprise when names like begin and size are in the global namespace. Get used to using the namespace prefix (std is intentionally very short), or importing just the names you need into the smallest reasonable scope.

The exceptions to this rule are namespaces explicitly intended to be imported wholesale, such as the std::literals namespaces.

Use tools to help

While this program appears to run correctly, running it under Valgrind reveals several memory leaks and uses of uninitialized/unallocated data:

 Invalid write of size 1
    at 0x4C2FE80: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x1099A8: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:52)
    by 0x109E63: main (189465.cpp:229)
  Address 0x5b34c80 is 0 bytes after a block of size 0 alloc'd
    at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
    by 0x109E63: main (189465.cpp:229)

 Invalid write of size 1
    at 0x4C2FE8D: strcpy (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x1099A8: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:52)
    by 0x109E63: main (189465.cpp:229)
  Address 0x5b34c85 is 5 bytes after a block of size 0 alloc'd
    at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
    by 0x109E63: main (189465.cpp:229)

...

 Invalid free() / delete / delete[] / realloc()
    at 0x4C2E7BB: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x109A0C: String::~String() (189465.cpp:64)
    by 0x10A59C: main (189465.cpp:229)
  Address 0x5b34cc0 is 0 bytes after a block of size 0 free'd
    at 0x4C2E7BB: operator delete[](void*) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x109A0C: String::~String() (189465.cpp:64)
    by 0x10A56F: main (189465.cpp:232)
  Block was alloc'd at
    at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
    by 0x109EB7: main (189465.cpp:230)

...

 HEAP SUMMARY:
     in use at exit: 100 bytes in 2 blocks
   total heap usage: 7 allocs, 7 frees, 73,828 bytes allocated

 0 bytes in 1 blocks are definitely lost in loss record 1 of 2
    at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x10997D: String::String(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (189465.cpp:51)
    by 0x109E63: main (189465.cpp:229)

 100 bytes in 1 blocks are definitely lost in loss record 2 of 2
    at 0x4C2D91F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
    by 0x109CA3: String::operator+(String const&) const (189465.cpp:162)
    by 0x10A40D: main (189465.cpp:264)

 LEAK SUMMARY:
    definitely lost: 100 bytes in 2 blocks
    indirectly lost: 0 bytes in 0 blocks
      possibly lost: 0 bytes in 0 blocks
    still reachable: 0 bytes in 0 blocks
         suppressed: 0 bytes in 0 blocks

 For counts of detected and suppressed errors, rerun with: -v
 ERROR SUMMARY: 444 errors from 78 contexts (suppressed: 0 from 0)

Document the design constraint

The assignment requirements seem to require us to hold only a pointer to start of string. Compared to std::string, which also has a member to hold the length of the string, this means our objects

  • will be smaller
  • cannot hold a string with embedded NUL
  • will have performance of O(n) rather than O(1) for any use of string length

Additionally, we do not have the std::string concept of capacity that's different from the string length, so every length-modifying operation must necessarily require allocation (well, we could shrink without reallocating, but we don't know how wasteful that might be).

If you want to improve your understanding, I recommend that you experiment further than the assignment requires - add a length member and see what you need to change to allow embedded NUL. You'll need to use std::memcpy() in place of std::strcpy() and std::strcat(), for instance.

Also, add a warning comment for the unconventional operator! that's required by the assignment - using ! to mean "size" is alien to C and C++ programmers, who expect it to be a boolean (true if the string is empty or otherwise "false" in some way).

Allocate enough memory for the NUL byte

This is one of the problems found by Valgrind, in the constructor:

    data = new char[0];
    data[0] = '\0';

If we want to write to data[0], we need to allocate at least 1 char (remember, the number in the the new[] expression is how many objects to allocate - the index of the last will be one less than the quantity):

    data = new char[1];
    data[0] = '\0';

I'd argue that it's better to use a member initializer to set data (and GCC can warn if you forget - that's helpful):

String::String()
    : data(new char[1])
{
    data[0] = '\0';
    countA();
}

Allocate enough memory for contents

In the std::string constructor, we need to allocate enough characters for the string we're copying, including its terminating NUL:

String::String(const std::string& thing)
    : data(new char[1+std::strlen(thing.c_str())])
{
    std::strcpy(data, thing.c_str());
    countA();
}

I haven't used thing.size() because thing might have embedded NULs, and our implementation will treat that as end of string.

Use appropriate type for sizes

A string might be longer than the range of int. The Standard provides std::size_t which will be big enough for anything that can fit in your program's address space, so we should use that for lengths, indexing and for our count of a characters.

Use the expected return types for operators

Relational operators return bool (those are already correct). Assignment operators should return a reference to the assigned-to object, and binary operators should return an instance of the common type of the arguments. That means:

String& operator = (const String& obj);
String& operator += (const String& obj);
String operator + (const String& obj) const;

Be responsible for allocated resources

This copy constructor has a serious problem:

String::String(const String &s2)
{
    data = s2.data;
    countA();
}

Suppose I have

{
    String a = "foo";
    String b = a;
}

At the end of the block, b is destructed, which will delete[] b.data. Then a is destructed, which will delete[] a.data. But a.data is the same as b.data, so we have a double delete!

What we need to do is to allocate new memory for b.data and copy the contents of a into it.

Construct from string literals

The test code provides string literals ("...") - there's no need to convert them into std::string for our constructor to just extract the character array. Instead we can construct directly from these:

String(char const*);

We can even make this act as default constructor, by providing a default argument:

String(const char * = "");

Don't provide a writeable view of our buffer

The assignment requires us to provide operator*, but we're over-generous with what we return. We can share the buffer in read-only form simply by changing its return type:

const char* operator * () const
{
    return data;
}

Now, the calling code gets a pointer to constant characters - which can't be written to without a cast.

Refactor adding and subtracting the counts

The only difference between countA() and lowerNumA is whether we add to or subtract from our variable. We can make a single function to count (and it can be private, as the existing methods should have been):

std::size_t String::count_a() const
{
    // know your algorithms - if we had an end iterator, we could
    // simply return std::count(begin, end, 'a')
    std::size_t count = 0;
    for (auto p = data;  *p;  ++p)
        count += (*p == 'a'); // no-one asked for 'A'
    return count;
}

Then we can replace calls to countA() and lowerNumA with numA += count_a(); and numA -= count_a(); respectively.

Don't forget the assignment operator

If we have to write a copy constructor, that normally means we need to write something similar in operator=() - except that first we need to release our existing contents (you can copy from the destructor).

(Exercise - does it work for self-assignment? If not, can you fix it?)

Reduce duplication in + and +=

Assigning and non-assigning binary operators come as pairs. Instead of having very similar code in two places, we can implement one in terms of the other.

Here's a first attempt:

String& String::operator += (String const &obj)
{
    auto prev = data;
    data = new char[1+std::strlen(prev)+std::strlen(obj.data)];
    std::strcpy(data, prev);
    delete[] prev;
    std::strcat(data, obj.data);
    numA += obj.countA();

    return *this;
}

// Not an efficient method, but simple and easy to get correct
// Consider implementing += as '*this = *this + other'
String String::operator + (String const &obj) const
{
    String s = *this;
    return s += obj;
}

(Exercise - what happens if we do a += a;? How can we fix the problem?)

We can make operator+ more efficient if we provide a constructor that takes two strings, so that it only allocates once, and implement += in terms of +:

String::String(String const &a, String const& b)
    : data(new char[1+std::strlen(a.data)+std::strlen(b.data)])
{
    std::strcpy(data, a.data);
    std::strcat(data, b.data);
    numA += countA();
}

String& String::operator += (String const &obj)
{
    return *this = *this + obj;
}

String String::operator + (String const &obj) const
{
    return String(*this, obj);
}

Complete the relational operators

If we have < and == operators, the rest can be defined in terms of them:

// member functions
bool String::operator < (String const &obj) const
{
    return std::strcmp(data, obj.data) < 0;
}

bool String::operator == (String const &obj) const
{
    return std::strcmp(data, obj.data) == 0;
}

// non-members
bool operator > (String const& a, String const& b)
{
    return b < a;
}

bool operator != (String const& a, String const& b)
{
    return !(a == b);
}

bool operator >= (String const& a, String const& b)
{
    return !(a < b);
}

bool operator <= (String const& a, String const& b)
{
    return !(b < a);
}

Alternatively, the non-members can be implemented with

using namespace std::rel_ops;

This is a namespace that's intended to be imported wholesale, unlike namespace std.


More advanced topics

Consider thread-safety

This code runs in a single thread, and that's fine. If we want to use our strings in different threads, we'll need to deal with concurrent access to the static numA variable. We could use a mutex, but this case is simple and common, so we can get all the benefit just by changing the type:

static std::atomic<std::size_t> numA;

Provide move-constructor and move-assignment

We can make code more efficient, by avoiding allocation when copying from temporary objects and other xvalue ("expiring") objects. These methods are easiest to implement in terms of swap (I recommend, but don't show, a swap() method instead of the simple std::swap() of members used here):

String::String(const String&& s2)
    : String()
{
    std::swap(data, s2.data);
}

String& String::operator = (String&& obj)
{
    std::swap(data, obj.data);
    return *this;
}

We don't need to do any allocation when we swap, and neither do we need to adjust the count, as we're just interchanging ownership of the buffers.


Further reading and exercises

A review that may be worth reading:

For more practice, in approximate order of difficulty:

  • Add a member to track the length of the string, as mentioned above in the review. Use this length to correctly copy and concatenate when the string has embedded null characters (you'll need to stop using std::strlen() and std::strcpy()/std::strcat() - you'll find std::memcpy() useful).
  • Manage capacity separately from length.
  • Add iterators, so we can use standard algorithms such as std::count() (see review Example of adding STL iterator support to custom collection class for some pitfalls.
  • Implement a view class, that exposes a substring of an existing string.

Modified code

#include <atomic>
#include <iostream>
#include <cstring>


class String
{
    static std::atomic<std::size_t> numA;

    char* data;  // no embedded NULs

    std::size_t countA() const;
    String(String const &a, String const& b); // for operator+

public:
    String(const char * = "");  // argument must not be null
    String(const String&);
    String(String&&);
    ~String();

    String& operator = (String);
    bool operator < (const String&) const;
    bool operator == (const String&) const;
    String& operator += (const String&);
    String operator + (const String&) const;
    std::size_t operator ! () const; // unconventional (but required by exercise)
    char operator[] (std::size_t) const;
    const char* operator * () const { return data; }

    friend std::ostream & operator << (std::ostream&, const String&);

    static std::size_t a_count() { return numA; }
};

String::String(const char *s)
    : data(new char[1+std::strlen(s)])
{
    std::strcpy(data, s);
    numA += countA();
}

String::String(const String &s2)
    : data(new char[1+std::strlen(s2.data)])
{
    std::strcpy(data, s2.data);
    numA += countA();
}

String::String(String&& s2)
    : String()
{
    std::swap(data, s2.data);
}

String::String(String const &a, String const& b)
    : data(new char[1+std::strlen(a.data)+std::strlen(b.data)])
{
    std::strcpy(data, a.data);
    std::strcat(data, b.data);
    numA += countA();
}

String::~String()
{
    numA -= countA();
    delete[] data;
}


std::size_t String::countA() const
{
    std::size_t count = 0;
    for (auto p = data;  *p;  ++p)
        count += (*p == 'a');
    return count;
}


// Group similar operators together
bool String::operator < (String const &other) const
{
    return std::strcmp(data, other.data) < 0;
}

bool String::operator == (String const &other) const
{
    return std::strcmp(data, other.data) == 0;
}

// non-member relational operators
// alternative: using namespace std::rel_ops;
bool operator > (String const& a, String const& b)
{
    return b < a;
}

bool operator != (String const& a, String const& b)
{
    return !(a == b);
}

bool operator >= (String const& a, String const& b)
{
    return !(a < b);
}

bool operator <= (String const& a, String const& b)
{
    return !(b < a);
}

// equivalent to destructor plus copy constructor
String& String::operator = (String other)
{
    std::swap(data, other.data);
    return *this;
}

String& String::operator += (String const &other)
{
    return *this = *this + other;
}

String String::operator + (String const &other) const
{
    return String(*this, other);
}

// N.B. this is the "length" operator, not negation
std::size_t String::operator ! () const
{
    return std::strlen(data);
}

char String::operator[] (std::size_t i) const
{
    return data[i];             // caller is expected to check range
}


std::atomic<std::size_t> String::numA = 0;
\$\endgroup\$
  • \$\begingroup\$ I would argue this needs to be changed. new char[1+std::strlen(thing.c_str())] and that the strcpy() in the same function needs be replaced with std::copy() to get around embedded nulls. \$\endgroup\$ – Martin York Mar 13 '18 at 16:27
  • \$\begingroup\$ I've updated the review, and included some comments (and an exercise) related to that constraint. std::memcpy() is as good as the more-general std::copy() for this (but std::strcpy() obviously no longer works). \$\endgroup\$ – Toby Speight Mar 14 '18 at 17:59
2
\$\begingroup\$
  1. You are using using namespace std;.
    That's a bad idea, read "Why is “using namespace std;” considered bad practice?".

  2. It's nice of you to allow constructing from a std::string. Still, there are many disadvantages to that, especially the way you do it:

    • You add a dependency on <string>. And you obviously forgot to add that header.
    • You don't support different char_traits and / or allocator.
    • Constructing from a string-literal depends on the detour through this constructor, which is not only horrendously inefficient but also explicitly disallowed in your instructions.

    Luckily, all the disadvantages are easily healed by moving to C++17 std::string_view, which is implicitly constructible from std::string and many other sources.

  3. Following the point above also allows you to make copy-construction more efficient. Instead of following std::strlen() with std::strcpy(), you can use the more efficient std::copy_n() or std::memcpy().

  4. Add a move-ctor and implement the friend-function swap(a, b) just to make things easier for you.

  5. You allocate the necessary memory in the ctor-body, instead of in the ctor-init-list. This means the dtor will be run while data is uninitialized if allocation fails. Fix that.

  6. Your assignment-operator is also an unmitigated disaster. You need to make a deep copy, meaning a copy of the contained string, instead of a shallow copy of the pointer. Don't forget to free the old string, make it self-assignment-safe, and remember that allocation can throw.
    Actually, rewrite it using the copy-and-swap-idiom.
    And remember that it should return *this by reference.

  7. operator+ should return a String, not an owning raw pointer. Also, don't assume that a char[100] suffices. Finally, why do you output an empty string in there, that's quite a costly no-op?

  8. operator+= should return *this by reference, like all assignment-operators. Your implementation simply assumes against all evidence that the array pointed to by data is big enough to accommodate concatenating the second string on the end. Unless the second string is empty, you write out-of-bounds, which is Undefined Behavior (UB). Fix that by reusing operator +.

  9. countA() and lowerNumA() should not be exposed, as they are implementation-details and must not be called from ouside, ever. Also, you can easily combine them.

  10. I suggest making the static data member a std::size_t. While that isn't quite guaranteed to be big enough if you create millions of your strings to hold the count of A's on all implementations, it avoids UB on overflow and those rare ones would probably also fail to provide std::uintptr_t.

  11. You may only create / destroy / append to instances of String in a single thread, as your static member is not accessed in a thread-safe manner. Luckily, you can easily remedy that using std::atomic, without having to introduce locks.

  12. Kudos for trying to provide the full complement of comparison operators. Still, you forgot <=, >= and !=.

  13. While there might be a reason to return the internal buffer from unary * (though the spec would allow returning *this, maybe by reference), you really must protect the internal buffer from modification.

  14. You can return a boolean expression directly. Branching on it and returning the corresponding literals in the branches is verbose and error-prone. Luckily, the compiler will not be confused so it isn't inefficient.

  15. You know std::strlen(), so why don't you use it in operator !?

  16. If you insist on checking bounds in operator[], throw an exception if you detect an out-of-bounds index. Don't just arbitrarily write to some stream and return something imaginary, as doing so makes re-use and debugging quite a lot harder.

  17. Putting every character individually in the stream is incredibly inefficient. Take advantage of the fact that you have a 0-terminated string and operator<< is overloaded for that.

  18. Consider following Almost Always auto where you can. Doing so removes needless clutter and repetition, promotes efficiency and diminishes headaches.

An implementation where everything is inline:

#include <string_view>
#include <cstring>
#include <ostream>
#include <algorithm>
#include <utility>
#include <atomic>

class String {
    char* data;
    auto countA() const noexcept {
        std::size_t n = 0;
        for (auto p = data; *p; ++p)
            n += *p == 'A' || *p == 'a';
        return n;
    }
    inline static std::atomic<std::size_t> numA;
    String(std::string_view a, std::string_view b)
    : data(new char[a.size() + b.size() + 1]) {
        std::copy_n(a.data(), a.size(), data);
        std::copy_n(b.data(), b.size(), data + a.size());
        data[a.size() + b.size()] = 0;
        numA += countA();
    }
public:
    String() : data(new char[1]) { *data = 0; }
    String(std::string_view o) : data(new char[o.size() + 1]) {
        std::copy_n(o.data(), o.size(), data);
        data[o.size()] = 0;
        numA += countA();
    }
    String(String&& o) noexcept : String() { swap(o, *this); }
    ~String() { numA -= countA(); delete [] data; }
    String& operator=(String&& o) { swap(*this, o); return *this; }
    String& operator=(std::string_view o) { swap(*this, String(o)); return *this; }

    operator std::string_view() const noexcept { return o.data; }
    friend void swap(String& a, String& b) noexcept { std::swap(a.data, b.data); }
    static std::size_t a_count() { return numA; }

    bool operator<(const String& o) const noexcept { return strcmp(data, o.data) < 0; }
    bool operator>(const String& o) const noexcept { return strcmp(data, o.data) > 0; }
    bool operator<=(const String& o) const noexcept { return strcmp(data, o.data) <= 0; }
    bool operator>=(const String& o) const noexcept { return strcmp(data, o.data) >= 0; }
    bool operator==(const String& o) const noexcept { return strcmp(data, o.data) == 0; }
    bool operator!=(const String& o) const noexcept { return strcmp(data, o.data) != 0; }

    char operator[](int i) const noexcept { return data[i]; }
    auto operator!() const noexcept { return std::strlen(data); }
    const char* operator*() const noexcept { return data; }
    friend auto& operator<<(std::ostream& o, const String& s) { return o << s.data; }
    auto& operator+=(const String& o) { return *this = *this + o; }
    auto operator+(const String& o) const { return String(*this, o); }
};
\$\endgroup\$
  • \$\begingroup\$ Looks like we found a lot of the same issues - good review! One thing I sometimes do is make member swap() return *this, so I can write String& operator=(String&& o) { return swap(o); }. Perhaps I'm taking lazy just that bit too far... \$\endgroup\$ – Toby Speight Mar 14 '18 at 18:09
  • \$\begingroup\$ @TobySpeight: Well, considering that member-swap is not all that useful to define at all, considering that generic code always uses non-member swap, as does most non-generic code... it seems quite harmless. \$\endgroup\$ – Deduplicator Mar 15 '18 at 13:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.