C++ string implementation error

Question

I am trying to implement a basic string class in C++. However, I am stuck on the append() function. It would like it to be Hello World, but it results as Hello ÍWorlýýýý««««««««þ:

#define __START namespace lib{
#define __END }

__START
class string
{
public:
    string(const char* s)
    {
        _str = s;
    }
    const char operator[](int position)
    {
        return _str[position];
    }
    void operator=(string s)
    {
        _str = s.c_str();
    }
    void append(string s)
    {
        size_t nlength = (length() + s.length());
        char* nstr = new char[nlength];
        for (int i = 0; i < length(); ++i)
        {
            nstr[i] = _str[i];
        }
        for (int i = length() + 1; i < nlength; ++i)
        {
            nstr[i] = s[(i - length() - 1)];
        }
        _str = const_cast<const char*>(nstr);
    }
    void operator+=(string s)
    {
        append(s);
    }
    const char* c_str()
    {
        return _str;
    }
    size_t length()
    {
        return strlen(_str);
    }
    std::string str()
    {
        return _str;
    }
private:
    const char* _str;
};
__END

int main()
{
    lib::string s = "Hello ";
    s.append("World"); // s += "World";
    std::cout << s.c_str();
    getchar();
}

By all means, use a different name for your string class. string conflicts with std::string. Of course you could have yournamespace::string, but it's better to avoid the confusion altogether. — Daniel Daranas
– Daniel Daranas, Commented Jan 28, 2014 at 18:04
@DanielDaranas And I would bet $1000 that OP is using namespace std; anyway. — user529758
– user529758, Commented Jan 28, 2014 at 18:05
You need to allocate nlength + 1 then add '\0' to end of string — 001
– 001, Commented Jan 28, 2014 at 18:05
Double underscores are reserved tokens in all contexts, leading underscores are reserved tokens in global scope, and leading underscores followed by capital letters are also reserved in all scopes. I would get rid of your #define preprocessor magic and just put the namespace scope inline. — Adam H. Peterson
– Adam H. Peterson, Commented Jan 28, 2014 at 18:08
Also, your constructor and operator= should make local copies. — 001
– 001, Commented Jan 28, 2014 at 18:09

ichramm · Accepted Answer · 2014-01-28 19:29:25Z

4

There are a lot of errors, not only with append

string(const char* s)
{
    _str = s;
}

The constructor is wrong, you should make a copy of s in order to free it later, this way:

~string()
{
    delete[] _str; // or free(_str) in case you use malloc/realloc, thanks Fred!
}

Private member variable:

private:
    const char* _str;

The internal string should not be const, you should be able to resize it later

const char operator[](int position)
{
    return _str[position];
}

You are missing a check: length() > position

void operator=(string s)
{
    _str = s.c_str();
}

You are not modifying s, it should be const string& s You are also not copying s.c_str() which means that now s and this are sharing the same internal buffer

void append(string s) // s should be a const reference too
{
    size_t nlength = (length() + s.length());
    char* nstr = new char[nlength];
    for (int i = 0; i < length(); ++i)
    {
        nstr[i] = _str[i];
    }
    for (int i = length() + 1; i < nlength; ++i)
    {
        nstr[i] = s[(i - length() - 1)];
    }
    _str = const_cast<const char*>(nstr);
}

Should be easier to write in terms of realloc:

void append(string s)
{
    int start = length();
    _str = realloc(_str, length() + s.length());
    for (int i = 0; i < s.length(); i++) {
        _str[start+i] = s[i];
    }
}

If you want to stick to new its OK, but you must free _str before assigning it to the new one.

The following operators should be const:

const char* c_str() const;
size_t length() const;
std::string str();

Update: Options for the constructor:

// option one (use delete[] to cleanup _str)
string(const char* s) {
    int n = strlen(s);
    _str = new char[n+1];
    memcpy(_str, s, n+1); // s is NULL terminated
}

// option two (use free() to cleanup _str)
string(const char* s) {
    int n = strlen(s);
    _str = (char*)malloc(n+1);
    memcpy(_str, s, n+1); // s is NULL terminated
}

// option 3: rely on append taking a char* argument
string(const char *s) : _str(NULL) {
    append(s, strlen(s));
}
..
void append(const string& s) {
    append(s.c_str(), s.length())
}
void append(const char *s, int len) {
  int start = _str ? length() : 0;
  _str = realloc(_str, start + len);
  for (int i = 0; i < len; i++) {
      _str[start+i] = s[i];
  }
}

Update 2: It will be better to use size_t or unsigned int instead of plain int because the size is always greater than or equal to zero.

edited Jan 28, 2014 at 19:29

answered Jan 28, 2014 at 18:19

ichramm

6,66425 silver badges32 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Fred Larson Over a year ago

Don't free something from new or delete something from malloc/realloc. Bad things could happen. See parashift.com/c++-faq/mixing-malloc-and-delete.html

ichramm Over a year ago

@FredLarson so true :)

Fred Larson Over a year ago

I said that because I'm very concerned about your recommendation of realloc, which would be bad when you hit that delete [] in the destructor.

ichramm Over a year ago

@FredLarson I know, I'm glad you mentioned it (you've got my upvote)

Joe Over a year ago

@ichramm - Thanks, how would I be able to make a copy of s?

Joseph · Accepted Answer · 2014-03-01 22:25:06Z

There are tons of problems with your code, so let's do this step by step.

First, you don't need your preprocessor magic for the namespace, but just a basic namespace mynamespace{}.

Secondly, it is ideal to create a basic_string class, so that it can be used with different character types. char / wchar_t, e.t.c.

Problems with your class:

1) private: const char *_str;. The pointer will be modified, so the const is useless.

2) There are no typedefs. You need them if you are attempting to re-implement a STL class. (Explained in example)

3) Use an allocator. This way you can construct, and destroy elements, allocate, and deallocate memory. You will be certain to keep your memory a lot safer this way.

4) Strings must be null terminated. That means an extra '\0' at the end, meaning you will have to allocate an extra byte for that. Strings are null terminated, because it is a way of telling the code to stop reading the string.

5) You are assigning to a string which has not been allocated. _str = s.c_str(); could easily crash, depending on your compiler, because you are writing to non-allocated memory.

6) Use const references, rather than normal types for your parameters (string = const string &). You also need this for your copy constructor basic_string(const _Myt &).

I still may not have highlighted all of the problems

Example basic_string class

template < typename _Elem, typename _Traits = std::char_traits<_Elem>, typename _Alloc = std::allocator<_Elem> > class basic_string
{
public:
    typedef basic_string<_Elem, _Traits, _Alloc> _Myt;
    typedef _Elem value_type;
    typedef _Traits traits_type;
    typedef _Alloc  allocator_type;
    typedef value_type *pointer;
    typedef const value_type *const_pointer;
    typedef value_type *iterator;
    typedef const value_type *const_iterator;
    typedef value_type &reference;
    typedef const value_type &const_reference;
    typedef std::size_t size_type;
    typedef std::ptrdiff_t difference_type;

    basic_string()
    {
        __data = _Alloc().allocate(1);
        _Alloc().construct(&__data[0], '\0');
    }

    basic_string(const_pointer _Init)
    {
        int count = 0;
        __data = _Alloc().allocate(_Traits::length(_Init) + 1);
        for (const_iterator i = &_Init[0]; i != &_Init[_Traits::length(_Init)]; ++i, ++count)
        {
            _Alloc().construct(&__data[count], *i);
        }
        _Alloc().construct(&__data[_Traits::length(_Init)], '\0');
    }

    basic_string(const _Myt &_Init)
    {
        if (this != &_Init)
        {
            int count = 0;
            __data = _Alloc().allocate(_Traits::length(_Init.__data) + 1);
            for (const_iterator i = &_Init.__data[0]; i != &_Init.__data[_Traits::length(_Init.__data)]; ++i, ++count)
            {
                _Alloc().construct(&__data[count], *i);
            }
            _Alloc().construct(&__data[_Traits::length(_Init.__data)], '\0');
        }
        else
        {
            __data = _Alloc().allocate(1);
            _Alloc().construct(&__data[0], '\0');
        }
    }

    ~basic_string()
    {
        if (__data)
        {
            size_type tmp = size();
            for (iterator i = begin(); i != end(); ++i)
            {
                _Alloc().destroy(i);
            }
            _Alloc().deallocate(__data, tmp);
        }
    }

    _Myt &assign(const_pointer _Rhs)
    {
        int count = 0;
        reserve(_Traits::length(_Rhs) + 1);
        for (const_iterator i = &_Rhs[0]; i != &_Rhs[_Traits::length(_Rhs)]; ++i, ++count)
        {
            _Alloc().construct(&__data[count], *i);
        }
        _Alloc().construct(&__data[_Traits::length(_Rhs)], '\0');
        return *this;
    }

    _Myt &operator=(const_pointer _Rhs)
    {
        return assign(_Rhs);
    }

    _Myt &append(const_pointer _Rhs)
    {
        int count = size();
        reserve(size() + _Traits::length(_Rhs) + 1);
        for (const_iterator i = &_Rhs[0]; i != &_Rhs[_Traits::length(_Rhs)]; ++i, ++count)
        {
            _Alloc().construct(&__data[count], *i);
        }
        _Alloc().construct(&__data[count], '\0');
        return *this;
    }

    _Myt &operator+=(const_pointer _Rhs)
    {
        return append(_Rhs);
    }

            iterator begin()
    {
        return &__data[0];
    }

    iterator end()
    {
        return &__data[size()];
    }

    size_type size()
    {
        return _Traits::length(__data);
    }

    _Myt &swap(basic_string<_Elem> &_Rhs)
    {
        std::swap(__data, _Rhs.__data);
        return *this;
    }

    void reserve(size_type _Size)
    {
        int count = 0;
        if (_Size < size())
        {
            return;
        }
        pointer buf = _Alloc().allocate(_Size);
        for (iterator i = begin(); i != end(); ++i, ++count)
        {
            _Alloc().construct(&buf[count], *i);
        }
        std::swap(__data, buf);
        for (iterator i = &buf[0]; i != &buf[_Traits::length(buf)]; ++i)
        {
            _Alloc().destroy(i);
        }
        _Alloc().deallocate(buf, _Traits::length(buf));
    }

    operator const_pointer()
    {
        return __data;
    }

    operator pointer()
    {
        return __data;
    }

    template < typename _Traits1, typename _Alloc1 > friend std::basic_ostream<_Elem> &operator<<(std::basic_ostream<_Elem> &_Stream, basic_string<_Elem, _Traits1, _Alloc1> &_Str)
    {
        return _Stream << _Str.c_str();
    }

    const_pointer data() const
    {
        return __data;
    }

    const_pointer c_str() const
    {
        return __data;
    }
private:
    pointer __data;
};

typedef basic_string<char> string;
typedef basic_string<wchar_t> wstring;

Of course, there is still a bit missing, but I'm sure you can implement that with a little online help.

barak manos · Accepted Answer · 2014-01-28 18:14:10Z

1

First of all, your most critical problem is in char* nstr = new char[nlength];.

You must change it to char* nstr = new char[nlength+1];.

Then, in function append, after the two for loops, set nstr[nlength] = 0;

Second, for better performance (as well as correct coding), you probably need to change string s to const string& s in the following functions:

void operator=(string s)
void append(string s)
void operator+=(string s)

answered Jan 28, 2014 at 18:14

barak manos

30.2k10 gold badges67 silver badges117 bronze badges

Comments

Mike Seymour · Accepted Answer · 2014-01-28 18:17:43Z

1

You have an off-by-one error in the second loop; the second string needs to be copied to length(), immediately after the end of the first:

for (int i = length(); i < nlength; ++i)
{
    nstr[i] = s[i - length()];
}

You'll also need to allocate one more byte to put a null terminator on the end.

Note that you don't need that scary-looking cast to add const to the pointer, since that's a perfectly safe thing to do. const_cast is only needed to remove qualifiers. You might also want to fix the memory leak and, for bonus points, cache the length so you don't have to read the entire string every time you want it.

edited Jan 28, 2014 at 18:17

answered Jan 28, 2014 at 18:12

Mike Seymour

256k30 gold badges467 silver badges658 bronze badges

Comments

Eric Fortin · Accepted Answer · 2014-01-28 18:10:56Z

0

for (int i = length() + 1; i < nlength; ++i)
{
    nstr[i] = s[(i - length() - 1)];
}

You first copy chars up to length()-1 and then start back at length() + 1, so you skip a char and there could be anything in there after allocation(unless you do a memset beforehand).

Then you need to terminate your string with a null character(\0) so that the std::string that gets constructed on str() method return knows when to stop reading chars.

answered Jan 28, 2014 at 18:10

Eric Fortin

7,6132 gold badges28 silver badges34 bronze badges

Collectives™ on Stack Overflow

C++ string implementation error

5 Answers 5

5 Comments

Comments

Comments

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

5 Answers 5

5 Comments

Comments

Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related