Holger Brunn wrote:

Sascha Volkenandt wrote:

...

Why would you want this, or better why should two cStrings point to the same buffer?

Thanks for your reply, after putting together an example, I found that my problem is rather a symptom. Look at this code:

#include "tools.h"

cString str=cString("hello world");

void func(cString string) { str=string; }

int main(int argc, char* argv[]) { printf("%s\n", *str); func(str); printf("%s\n", *str); }

The problem is that str and string in func point to the same buffer. And even without assinging string to str, the second printf receives a freed buffer, for cString's destructor will be called for string when func returns.

Then apart from dealing with the same-buffer thing, shouldn't cString have a copy constructor to take care of duplicating the buffer for this case? Or is cString intended to be passed by reference only?

Thanks for comments Holger

A copy ctor would certainly be a good idea. Please send a patch and let us know if that fixes your problem.

Klaus

Holger Brunn wrote:

cString &cString::operator=(const cString &String) {

• free(s);

• if(s!=String.s)
• {
• free(s);
• } s = String.s ? strdup(String.s) : NULL; return *this;

}

Doesn't this cause a memory leak? It'll strdup() the old string and then lose the old pointer to it. Looks to me like it should instead be:

if(s!=String.s) { free(s); s = String.s ? strdup(String.s) : NULL; }

or maybe something like: if (&String == this) return *this;

Jon

Holger Brunn wrote:

Jon Burgess on Sunday 20 November 2005 18:17:

...

Doesn't this cause a memory leak? It'll strdup() the old string and then lose the old pointer to it.

Hello, as far as I can see, there wouldn't be a mem leak - as both cStrings point to the same buffer, you only loose one of them. The one that was assigned from still has a pointer to the original buffer (and its destructor will free it).

In the normal case you'd be correct, the destructor would be called twice and free up both, but in the case that the cStrings are equal the destructor will only be called once on the new cString. There is nothing which will destruct the old cString. I wrote the quick test app as attached and valgrind seems to agree with me.

[jburgess@shark cstring]$ g++ -g -O2 -Wall -o main main.c [jburgess@shark cstring]$ ./main operator= called Hello World [jburgess@shark cstring]$ valgrind --tool=memcheck ./main ==18765== Memcheck, a memory error detector. ==18765== Copyright (C) 2002-2005, and GNU GPL'd, by Julian Seward et al. ==18765== Using LibVEX rev 1367, a library for dynamic binary translation. ==18765== Copyright (C) 2004-2005, and GNU GPL'd, by OpenWorks LLP. ==18765== Using valgrind-3.0.1, a dynamic binary instrumentation framework. ==18765== Copyright (C) 2000-2005, and GNU GPL'd, by Julian Seward et al. ==18765== For more details, rerun with: -v ==18765== operator= called Hello World ==18765== ==18765== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 9 from 4) ==18765== malloc/free: in use at exit: 12 bytes in 1 blocks. ==18765== malloc/free: 2 allocs, 1 frees, 24 bytes allocated. ==18765== For counts of detected errors, rerun with: -v ==18765== searching for pointers to 1 not-freed blocks. ==18765== checked 196608 bytes. ==18765== ==18765== LEAK SUMMARY: ==18765== definitely lost: 12 bytes in 1 blocks. ==18765== possibly lost: 0 bytes in 0 blocks. ==18765== still reachable: 0 bytes in 0 blocks. ==18765== suppressed: 0 bytes in 0 blocks. ==18765== Use --leak-check=full to see details of leaked memory.

Jon

Holger Brunn wrote:

I see, didn't think of this. So we either need also a check for equality of references or just forget about it completely, as both situations would be rather theoretical. So could we agree on

if(&String == this) return *this; if(s!=String.s) { free(s); } s = String.s ? strdup(String.s) : NULL;

This will avoid the memory leak you pointed out and beahve the way you would expect from an assignment operator?

What happens if someone was trying to remove the initial part of the string, e.g. something like:

cString hw("Hello World"); hw = strstr(*hw, " ")+1;

The two buffers wouldn't be equal, but freeing the input before the strdup would still be wrong. How about:

if(&String == this) return *this; const char *old = s; s = String.s ? strdup(String.s) : NULL; free(old);

Does this stll match your expected behaviour?

Jon

Sascha Volkenandt wrote:

On Sunday 20 November 2005 20:53, Jon Burgess wrote:

[ explaination of why the strstr example works deleted ]

So, no unexpected behaviour here.

Yes, you're right. The intermeadiate buffer which is created avoids the problem. I confirmed this myself after sending the previous email.

Like I mentioned before, I don't think all of this is necessary since the normal ctor and the copy ctor cover all cases.

I think something should be done to fix the "assign cString to itself" case, even though this is very unlikely. This does go wrong with the current code, even with the copy constructor added.

Jon

On Tuesday 22 November 2005 10:33, Holger Brunn wrote:

Okay, now as all seems to be said, here is a patch that checks for that case (that may happen when working with references/pointers to cStrings), leaving Klaus with the decision whether it's useful to have it or not.

Is this the complete patch now? I still suggest adding a copy ctor (which should word like operator= without freeing)..

Greetings, Sascha

Holger Brunn holger.brunn@stud.uni-karlsruhe.de wrote:

...

I think something should be done to fix the "assign cString to itself" case, even though this is very unlikely. This does go wrong with the current code, even with the copy constructor added.

Okay, now as all seems to be said, here is a patch that checks for that case (that may happen when working with references/pointers to cStrings), leaving Klaus with the decision whether it's useful to have it or not.

why not just:

typedef std::string cString;

or:

class cString : public std::string { ... };

i really can't understand all this messing with a datatype allready implemented elsewhere. what is so special about cString that other string implementations do not have?

my 0.02E clemens

On Sunday 20 November 2005 13:18, Holger Brunn wrote:

Adding the copy constructor fixes my problem by avoiding it. So I made two patches, both with the copy constructor, one also has a check for equal buffers in operator= and doesn't free them in this case.

Would you mind to explain how you get two strings pointing to the same buffer after the copy ctor is safe (makes a copy instead of taking the buffer)? IMHO the copy ctor already takes care that doesn't happen (except if you hand over the same buffer to two cStrings with TakePointer = true, which is IMHO not intended).

Greetings, Sascha

On Sunday 20 November 2005 22:07, Jon Burgess wrote:

Sascha Volkenandt wrote:

...

Would you mind to explain how you get two strings pointing to the same buffer after the copy ctor is safe (makes a copy instead of taking the buffer)? IMHO the copy ctor already takes care that doesn't happen (except if you hand over the same buffer to two cStrings with TakePointer = true, which is IMHO not intended).

As you say, all normal usage should be between 2 different buffers. It doesn't seem legal to me that 2 cStings would share the same buffer. This would always cause things to blow up when the destructors are called.

The only way that I can come up with is to assign a cString to itself (as per my example main.c elsewhere in this thread). I imagine this could happen legally under some limited circumstances.

I don't know the details of the scenario seen by Holger.

Jon

hi, sometimes this is done with a sense.

if you have a look at the standard template library, you'll find out that the strings - exactly the content - is shared by refcounting and a copy is just done, if it is needed.

i had a short look at the tools.c/h files and closed them fast again. imho mixing up c & c++ permanently is not a good style at all.

have a look at std::basic_string. it is working with copy_on_write.

just my 2ct

marcel

On Sunday 20 November 2005 12:35, Holger Brunn wrote:

Sascha Volkenandt wrote:

...

Why would you want this, or better why should two cStrings point to the same buffer?

Thanks for your reply, after putting together an example, I found that my problem is rather a symptom. Look at this code:

#include "tools.h"

cString str=cString("hello world");

void func(cString string) { str=string; }

int main(int argc, char* argv[]) { printf("%s\n", *str); func(str); printf("%s\n", *str); }

The problem is that str and string in func point to the same buffer. And even without assinging string to str, the second printf receives a freed buffer, for cString's destructor will be called for string when func returns.

Then apart from dealing with the same-buffer thing, shouldn't cString have a copy constructor to take care of duplicating the buffer for this case? Or is cString intended to be passed by reference only?

You're absolutely right, I didn't think of such a scenario first, Apart from the fact that a call by const-reference would reduce overhead in this case, there are similar situations where that doesn't apply.

I've stumbled over one recently, but I solved it by surrounding it because I didn't think Klaus wanted to blow up the cString class any further. But now that Klaus almost accepted it already, I think it's a very good idea to implement a copy ctor :-).

Greetings, Sascha