deleting variable

Hi!

I'm a little confused about deleting a variable in this case:

file String.hpp ...

Code:

...
class String {
  public:
	String(const char *text);
	~String();
	...
  private:
	void setString(const char *text);
	void freeMem(void *mem);

	char *text;
};

file String.cpp ...

Code:

# include ".\String.hpp"
...

String::String() {
  text = new char('\0');
}

String::~String() {
  freeMem(text);
}

void String::freeMem(void *mem) {
  delete mem;
  mem = NULL;
}

void String::setString(const char *text) {
  // delete previously allocated memory
  freeMem(text);

  // allocate a new memory
  int len = (int)strlen(text);
  this->text = new char[len + 1];
  strncpy(this->text, text, len + 1);
}
...

Testing the String class
file main.cpp ...

Code:

# include ".\String.hpp"

int main() {
  char *myText = new char[21];

  strncpy(myText, "hello world!", 21);
  String s = myText;
  delete myText;	// IS THIS CORRECT?
  myText = NULL;  // if not, than where should I clear the memory?
  cout << s;
  return 0;
}

Otherwise, is this design good?

You don't need to do most of that. Testing it would be as simple as:

Code:

String s = "Hello world";
cout << s;
s = "Hello world 2!";
cout << s;

There is no need for the dynamic memory allocation/deletion of characters in the test program.

As far as the design goes, there are plenty of things you could do to make it better and faster. if you're looking to make your own string class as practice for something, then perhaps try and implement one that supports the use of UNICODE characters as well as ANSI. You could do something smarter than deleting and reallocating every time you assign a value to the string.

As a side note, this is what your freeMem function should look like:

Code:

void String::freeMem(void *mem) {
  delete [] mem;
  mem = NULL;
}

... and your constructor should look like this:

Code:

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

Hope that helps!

You must match each new[] with a delete[]. Thus, every allocation in String should use new[], because otherwise you have to remember which you used.

Using strncpy is not necessary in strcpy. It is only necessary if the input string might be longer than the destination, or might be missing a terminating NUL. Neither is the case here: if it was missing a NUL, strlen already read past its end. And since you use strlen, you're guaranteed to have sufficient space.

Don't cast the return value of strlen. Rather, make len a size_t.

There's no need in main to ever allocate this buffer. Just assign the string literal directly.

Thanks for the reply.

Can you give me an example for using UNICODE?

Basically, you'd replace all occurences of char with wchar_t, all occurrences of strlen with wcslen etc. However, to make it work with both is trickier. You'd have to make your class a template on the character type. (This is how std::string works. It's a class template called basic_string, where string is a typedef for char and wstring one for wchar_t.) However, when you've done that, you can't use strlen and friends anymore, so you need a traits template that defines how to find the length of a nul-terminated sequence of that type and similar things. (std::string uses std::char_traits. You can just reuse the interface.) That traits type would be the second template parameter and should have some default. (std::char_traits, for example.)

Actually, your freeMem function should look like this:

Code:

void String::freeMem(void *&mem) {
  delete [] mem;
  mem = NULL;
}

Actually, it should take no parameter at all and work directly on the text member. Nice catch, though.

What does this mean "void *&mem"? What's the trick?

Well, the problem is that in freeMem, you set to NULL a local copy of the pointer. So, while you actually free the memory pointed to, the "text" member of the class still points to that - now invalid - memory.
MrWizard's weird syntax means that a reference to the pointer will be passed, not a copy of it. Thus, when you set it NULL, the actual "text" member will be set to NULL.

I say it's better to remove the parameter altogether and act directly on the "text" member.