-1

I get a problem with my String class.

Constructor and copy constructor:

private:
    char * buf;
public:
    String( const char * s = "")
    {
        buf = strdup(s);
    }
    String( String &s)
    : buf(strdup(s.buf))
    {
    }

The operator:

String operator + ( const String &s )
{
    char* tmp = new char[strlen(buf) + strlen(s.buf) + 1]; // +1 for the terminating '\0'
    strcpy(tmp,buf);
    strcat(tmp,s.buf);
    String result;
    delete[] result.buf;
    result.buf = tmp;
    return result;
}
String operator += ( const String s )
{
    delete_char_array(buf);
    char* tmp = new char[strlen(buf) + strlen(s.buf) + 1];
    strcpy(tmp,buf);
    strcat(tmp,s.buf);
    buf = strdup(tmp);
    delete_char_array(tmp);
    return *this;
}
void print( ostream & out )
{
    char *p = buf;
    out << p;
}

and I have implemented the << operator

ostream & operator << ( ostream & out,  String str )
{
    str.print(out);
    return out;
}

My main() function is:

int main()
{
    String firstString("First");
    String secondString("Second");
    cout << firstString + secondString << endl;
    (firstString+=secondString).print(cout);
}

I can correctly get the output by the String.print(cout) but the g++ will tell me that

cout << firstString + secondString << endl;` does not have the matching
constructor for `String::String(String)`

and there are two options, 1. String::String( char * c) and 2. String::String(String &s).

LogicStuff
  • 19,397
  • 6
  • 54
  • 74
Ruidongd
  • 41
  • 5
  • 2
    Use `const` in the copy constructor: `String(const String &s)` – πάντα ῥεῖ Feb 03 '16 at 21:28
  • 2
    You're using `strdup` in your constructor, and then you mix in `new[]` in your other functions. So what is your destructor going to look like? The `strdup` returns a pointer that is deallocated using `free()`, not `delete[]`. Why didn't you just use `new[]` in the constructor also? – PaulMcKenzie Feb 03 '16 at 21:38
  • 2
    Also, operator `+=` should be returning a reference to the current object, not a brand new String object. In addition, `operator +` can be written in terms of `operator +=` -- there is no need for code duplication. In short, there are a few more issues with your class besides the copy constructor. – PaulMcKenzie Feb 03 '16 at 21:48

2 Answers2

3

This should be the copy constructor signature:

String(String const& s)

so the reference can bind to an r-value returned by operator+. Additionally, you should also use this signature for operator<<:

ostream &operator<<(ostream &out, String const& str)

Any of these snippets will fix the error, however (1) is a crucial practice (as you see) and (2) avoids making a copy.

You'll also need to make void print(ostream &out) const, because you want to call it with String const&, even operator+ should be const. Please, don't forget the const...

You don't modify a reference - make it a reference-to-const. A member function does not modify *this - make it const. It's that simple.

You also forgot an & here: String operator+=(const String s).

We're still not finished, there's also a number of things to fix, as pointed out in PaulMcKenzie's comments.

LogicStuff
  • 19,397
  • 6
  • 54
  • 74
  • After adding the const in operator <<, the compiler tells me that member function 'print' not viable: 'this' argument has type 'const String', but function is not marked const – Ruidongd Feb 03 '16 at 21:33
  • 4
    `operator +=` should really be returning a `String&` and then the return would be `return *this;`. – PaulMcKenzie Feb 03 '16 at 21:49
  • @PaulMcKenzie Thanks, I've added a reference to avoid making a copy :) – LogicStuff Feb 03 '16 at 21:55
1

There are a few things wrong with your String class.

First, usage of strdup returns an allocated buffer that should be deallocated using free(). However in your overloaded += and + functions, you're using new [] and delete []. You're using C++, so pretend that strdup() doesn't exist.

Even though you're trying to be careful about it, it is undefined behavior to mix allocation functions like this. Instead, use new [] and delete [] throughout your class code.

Second, the copy constructor signature should be:

    String(const String &s);

So your constructors should now look like this:

private:
    char * buf;
public:
    String( const char* s = "") : buf(new char[strlen(s) + 1]())
    {
        strcpy(buf, s);
    }

    String(const String &s) : buf(new char[strlen(s.buf) + 1]())
    {
        strcpy(buf, s.buf);
    }

You now need to add a destructor, if you haven't done so already:

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

Next is your operator += and operator +. The operator += should be returning a reference to the current object, not a brand new object.

String& operator += ( const String& s )
{
    char* tmp = new char[strlen(buf) + strlen(s.buf) + 1];
    strcpy(tmp,buf);
    strcat(tmp,s.buf);
    delete [] buf;
    buf = tmp;
    return *this;
}

Note that the return type is a reference to String. Also, note that the memory for the concatenated string is allocated first before any attempt is made to delete the old buffer. This needs to be done to ensure that your buffer will not become invalidated if operator new throws an exception.

Given this, operator + can be written this way:

String operator + ( const String& s )
{
    String tmp(*this);
    return tmp += s;
}

We use operator += as a helper function. All that was done was to create a temporary String that was the same as *this. Then we just return the result of operator += on the passed in String. Short and simple.

The last thing you're missing is an assignment operator. Without an assignment operator, you cannot safely do things like this:

String s1("abc");
String s2;
s2 = s1; // <-- assignment operator here

To implement an assignment operator is very easy, provided you have a copy constructor and working destructor. Please go to this link as to how to implement this function:

copy / swap idiom.

Community
  • 1
  • 1
PaulMcKenzie
  • 34,698
  • 4
  • 24
  • 45