C++ Question - itoa, variadic functions, dynamic memory, etc.

[monkey0506]

As a bit of coding practice, I've been playing around with making my own string class for C++. For the most part it will be based off of the std::string class's functionality, but I'm trying to include a formatting function that works like AGS's String.Format (except that, like the other std::string functions, it does modify the string).

However, I've apparently ran into some sort of memory management issues or something as my program keeps crashing.  This is what I'm doing:

Code: ags

#ifndef __STRING_H
#define __STRING_H 1
#ifndef _GLIBCXX_IOSTREAM
#include <iostream>
#endif
#ifndef _STDARG_H
#include <stdarg.h>
#endif
#ifndef _STDLIB_H_
#include <stdlib.h>
#endif

class String {
  protected:
    char* __buffer;
  public:
    typedef unsigned int size_type;
    String();
    String(const char letter);
    String(size_type char_count, const char letter);
    String(const char* newstr, size_type index = 0, size_type length = 0);
    String(const String& newstr, size_type index = 0, size_type length = 0);
    ~String();
    size_type find(const char letter) const;
    size_type find(const char* str, size_type index = 0, size_type length = 0) const;
    size_type find(const String& str, size_type index = 0, size_type length = 0) const;
    String& format(const char* fmt, ...);
    String& format(const String& fmt, ...);
    String& replace(size_type index, size_type length, const char letter);
    String& replace(size_type index, size_type length, const char* str);
    String& replace(size_type index, size_type length, const String& str);
    static inline const size_type length(const char* str);
    inline const size_type length() const;
    friend std::ostream& operator<<(std::ostream& __out, const String& str);
    static const size_type npos = 4294967295U;
  };

String::String() {
  delete [] __buffer;
  __buffer = new char('\0');
  }

String::String(const char letter) {
  delete [] __buffer;
  __buffer = new char(letter);
  *(__buffer + 1) = '\0';
  }

String::String(const char* newstr, size_type index, size_type length) {
  delete [] __buffer;
  if ((!String::length(newstr)) || (index >= String::length(newstr))) {
    __buffer = new char('\0');
    return;
    }
  if ((!length) || (length > String::length(newstr))) length = String::length(newstr) - index;
  __buffer = new char[length];
  for (size_type i = 0, j = index; i < length; i++, j++)
    *(__buffer + i) = *(newstr + j);
  *(__buffer + length) = '\0';
  }

String::String(const String& newstr, size_type index, size_type length) {
  delete [] __buffer;
  if ((!newstr.length()) || (index >= newstr.length())) {
    __buffer = new char('\0');
    return;
    }
  if ((!length) || (length > newstr.length())) length = newstr.length() - index;
  __buffer = new char[length];
  for (size_type i = 0, j = index; i < length; i++, j++)
    *(__buffer + i) = *(newstr.__buffer + j);
  *(__buffer + length) = '\0';
  }

String::size_type String::find(const char letter) const {
  for (size_type i = 0; i <= length(); i++) if (*(__buffer + i) == letter) return i;
  return npos;
  }

String::size_type String::find(const char* str, size_type index, size_type length) const {
  for (size_type i = 0; i <= String::length(); i++)
    if (*(__buffer + i) == *str)
      for (size_type j = i, k = 0; ; j++, k++) {
        if (k == String::length(str)) return i;
        if ((j > String::length()) || (*(__buffer + j) != *(str + k))) break;
        }
  return npos;
  }

String::size_type String::find(const String& str, size_type index, size_type length) const {
  for (size_type i = 0; i <= String::length(); i++)
    if(*(__buffer + i) == *str.__buffer)
      for (size_type j = i, k = 0; ; j++, k++) {
        if (k == str.length()) return i;
        if ((j > String::length()) || (*(__buffer + j) != *(str.__buffer + k))) break;
        }
  return npos;
  }

String::size_type string_format_get_index(String& fmt) {
  if ((fmt.find("%d") == String::npos) && (fmt.find("%s") == String::npos) &&
  (fmt.find("%f") == String::npos)) return String::npos;
  String::size_type index = fmt.find("%d");
  if (fmt.find("%s") < index) index = fmt.find("%s");
  if (fmt.find("%f") < index) index = fmt.find("%f");
  return index;
  }

String& String::format(const char* fmt, ...) {
  // support %d for ints, %s for Strings, and %f for floats
  *this = fmt;
  if ((find("%d") == npos) && (find("%s") == npos) && (find("%f") == npos))
    return *this;
  va_list ap;
  va_start(ap, fmt);
  for (size_type index = string_format_get_index(*this); index != npos; index = string_format_get_index(*this)) {
    char type = (*this)[index + 1];
    char* temp;
    if (type == 'd') {
      replace(index, 2, itoa(va_arg(ap, int), temp, 10));
      delete [] temp; // this is to try and make sure the string isn't stored in dynamic memory as I'm not sure how itoa works....
      }
    if (type == 's') replace(index, 2, va_arg(ap, char*));
    if (type == 'f') {
      // no support for adding floats to Strings yet!
      }
    }
  va_end(ap);
  return *this;
  }

String& String::replace(size_type index, size_type length, const char letter) {
  if (index >= String::length()) return *this;
  if ((!length) || (length > String::length())) length = String::length() - index;
  char temp[(String::length() - length) + 1];
  for (size_type i = 0; i < index; i++)
    *(temp + i) = *(__buffer + i);
  *(temp + index) = letter;
  for (size_type i = index + 1, j = index + length; j <= String::length(); i++, j++)
    *(temp + i) = *(__buffer + j);
  delete [] __buffer;
  __buffer = new char[String::length(temp)];
  for (size_type i = 0; i <= String::length(temp); i++)
    *(__buffer + i) = *(temp + i);
  return *this;
  }

String& String::replace(size_type index, size_type length, const char* str) {
  if (index >= String::length()) return *this;
  if ((!length) || (length > String::length())) length = String::length() - index;
  char temp[(String::length() - length) + String::length(str)];
  for (size_type i = 0; i < index; i++)
    *(temp + i) = *(__buffer + i);
  for (size_type i = index, j = 0; j < String::length(str); i++, j++)
    *(temp + i) = *(str + j);
  for (size_type i = index + String::length(str), j = index + length; j <= String::length(); i++, j++)
    *(temp + i) = *(__buffer + j);
  delete [] __buffer;
  __buffer = new char[String::length(temp)];
  for (size_type i = 0; i <= String::length(temp); i++)
    *(__buffer + i) = *(temp + i);
  return *this;
  }

String& String::replace(size_type index, size_type length, const String& str) {
  if (index >= String::length()) return *this;
  if ((!length) || (length > String::length())) length = String::length() - index;
  char temp[(String::length() - length) + str.length()];
  for (size_type i = 0; i < index; i++)
    *(temp + i) = *(__buffer + i);
  for (size_type i = index, j = 0; j < str.length(); i++, j++)
    *(temp + i) = *(str.__buffer + j);
  for (size_type i = index + str.length(), j = index + length; j <= String::length(); i++, j++)
    *(temp + i) = *(__buffer + i);
  delete [] __buffer;
  __buffer = new char[String::length(temp)];
  for (size_type i = 0; i <= String::length(temp); i++)
    *(__buffer + i) = *(temp + i);
  return *this;
  }

inline const String::size_type String::length(const char* str) {
  if (!str) return 0;
  for (size_type i = 0; ; i++) if (*(str + i) == '\0') return i;
  }

inline const String::size_type String::length() const {
  return length(__buffer);
  }

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

std::ostream& operator<<(std::ostream& __out, const String& str) {
  return (__out << str.__buffer);
  }

#endif

I've also overloaded all the appropriate operators...if you want those just ask. I figured this code was long enough as it was.

Anyway, I can format strings like this:

Code: ags

String str;
str.format("this is some %s that has been %s", "text", "formatted");

And the like. However, if I try to insert integers:

Code: ags

String str;
str.format("the answer is %d", 42);

The program terminates...somewhere after properly formatting the string, but before it manages to return....

Just curious if anyone had any ideas on what I could do to resolve this problem.

[Kweepa]

Run it in a debugger?
I see a few problems:
* deleting something that hasn't been allocated (in the constructors)
* newing rather than new[]ing in the constructor (the destructor delete[]s)
* running over the end of allocated memory (in the second constructor)

[EDIT]
You need to allocate memory for the string that gets filled in by itoa.

Code: ags

char temp[33];
itoa(va_arg(ap, int), temp, 10);

http://msdn2.microsoft.com/en-us/library/yakksftt.aspx

[Shane 'ProgZmax' Stevens]

I don't really have time to look at the code, but did you try converting the integer to a string with itoa() and working with it that way?

[monkey0506]

The reason I deleted the memory in the constructors is because deleting 0 doesn't do anything, however there are certain situations where the constructors are called while the String actually has memory allocated to it.

Using "delete []" with a non-array works the same as just using "delete"...from what I understand...does it not?

And yes, I noticed that the one constructor does access memory outside of the appropriate range, however I never use that constructor (I'll fix it...just saying that isn't (currently) adding to the problem (therefore isn't causing it)).

---------------

Prog, I am using itoa.

[Kweepa]

The reason I deleted the memory in the constructors is because deleting 0 doesn't do anything, however there are certain situations where the constructors are called while the String actually has memory allocated to it.

It's undefined behaviour and shouldn't be done - in "release" the variable won't be zeroed and the delete will crash.
There's no way that your String can have memory allocated to it, since it's being constructed and didn't exist before.

Using "delete []" with a non-array works the same as just using "delete"...from what I understand...does it not?

Nope, it will attempt to delete an array of memory allocations, which may crash.

Prog, I am using itoa.

Check my edited post above.

[Radiant]

I'm trying to include a formatting function that works like AGS's String.Format (except that, like the other std::string functions, it does modify the string).

Are you familiar with sprintf() ?

[Kweepa]

The problem with sprintf in this context is that you don't know what size the buffer should be without pretty much doing the work of sprintf in the first place. The advantage of course is that it's feature complete and completely debugged.

[monkey0506]

I'm trying to include a formatting function that works like AGS's String.Format (except that, like the other std::string functions, it does modify the string).

Are you familiar with sprintf() ?

As a bit of coding practice, I've been playing around with making my own string class for C++.

Yes, I've come across it...

Steve, I added checks to the constructors (if (__buffer) delete [] __buffer;) and I've corrected the problem with filling __buffer with a single char instead of a char[] like it should.  Also I didn't know that I was required to allocate the memory needed for itoa, so thanks. I'll see what I can do with that info.

[EDIT:]

Thanks Steve...that fixed the problem.  However...is there any way to find out the size of the string it's going to return?

[Kweepa]

Steve, I added checks to the constructors (if (__buffer) delete [] __buffer;)

There is no way that __buffer can be valid in the constructor, so you shouldn't try to delete it - instead you should just initialize it to NULL, preferably with an initializer list.

Thanks Steve...that fixed the problem.  However...is there any way to find out the size of the string it's going to return?

As the msdn documentation says, it can be up to 33 characters. Best to just allocate it on the stack as I did in my code snippet, then copy it to wherever you need.

[monkey0506]

Hmm...my float-appending code is appending some junk values at the end of the String...

Code: ags

if (type == 'f') {
      double value = va_arg(ap, double);
      int ivalue = static_cast<int>(value);
      String stemp = itoa(ivalue, temp, 10);
      stemp += '.';
      stemp += itoa(static_cast<int>((value - ivalue) * 1000000), temp, 10);
      replace(index, 2, stemp);
      }

There's a loss of precision, but that's common amongst floating point values as I've been told...and I used "double" instead of "float" because my compiler told me it gets upgraded when it's passed through "...".

[Kweepa]

This would seem to be a good place to use sprintf.