Re: cString operator= with same buffer

  Klaus Schmidinger on Sunday 20 November 2005 12:41:
> A copy ctor would certainly be a good idea.
> Please send a patch and let us know if that fixes
> your problem.

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.

Greetings
Holger
  

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
  

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).

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

This is possibility 1) from my original posting I asked for arguments for. As 
stated there, I'd prefer making a new copy as I expect operator= to do so in 
every case.

Greetings
Holger
  

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
  

Jon Burgess on Sunday 20 November 2005 20:01:
> 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.

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?

Greetings
Holger
  

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
  

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 20:53, Jon Burgess wrote:
> 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;

In that case the following would happen (with the original cString plus a 
simple copy ctor):
1. strstr gets the "second word" of "Hello World" (which is part of hw's 
buffer)
2. a temporary cString is constructed with that result (because cString has no 
operator=(const char*), only an operator=(const cString&) ) making the 
"second word" a copy of the second half of hw's buffer
3. operator=(const cString&) frees hw's old buffer (which is not referenced by 
the temporary) and replaces it with a copy of the temporary cString's buffer
4. the temporary cString is destructed freeing it's buffer

So, no unexpected behaviour here.

Like I mentioned before, I don't think all of this is necessary since the 
normal ctor and the copy ctor cover all cases. Besides, I've never 
encountered string classes doing such overly complicated things to handle 
their buffers, including robust frameworks such as QT or MFC.

Greetings,
Sascha
  

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
  

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
  

DXR3 plugin support NTSC material yet?  There are buckets of these cards 
around here, and was hoping to use some :)


Thanks!
  

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