[interfacekit] Re: BString patch suggestion and some questions, too

> Hi everyone,
Hi Oliver !

> I am using a fork of the OBOS-BString class for Beam and I have found some 
> peculiarities with the given implementation:

Hmmm.... I haven't looked at that code for so much time... let me get back on 
track....
ok...

BTW... Why a fork ? :PPPP


> 1) one of the (many) tests is IMHO incorrect, since it seems to rely on
>    malloc/realloc setting memory to '\0' (which it doesn't).
>    Here's the code (from src/tests/.../bstring/StringAccessTest.cpp,72):
> 
>               NextSubTest();
>               BString emptylocked;
>               ptr = emptylocked.LockBuffer(10);
>               CPPUNIT_ASSERT(strcmp(ptr, "") == 0);
>                       // this test fails ^
>               strcat(ptr, "pippo");
>               emptylocked.UnlockBuffer();
>               CPPUNIT_ASSERT(strcmp(emptylocked.String(), "pippo") == 0);
> 
>    The call to LockBuffer returns a buffer of 11 bytes (with the last one
>    being initialized to '\0'). Since the start of this buffer is not  
>    initialized, strcmp fails most of the time (it does for me!).
>    I suggest to simply remove the first strcmp(), as it doesn't make sense.

Yes, you are probably right. This check : "strcmp(ptr, "") == 0" would maybe 
make sense if it's done before the LockBuffer call, but that case is already 
tested in another test, so we can remove it.
 
> 2) memory-allocations aren't checked for failure at all, so BString will   
>    just crash and burn in case of memory-shortage.

And this is really bad. My fault :)

>    My suggestion'd be to check each malloc/realloc and leave _privateData
>    NULL if the memory couldn't be allocated. Furthermore, the result of
>    _GrowBy(), ShrinkAtBy() and _OpenAtBy() should be checked like this:
> 
>               BString&
>               BString::Append(char c, int32 count)
>               {
>                       int32 len = Length();
>                       if (_GrowBy(count))
>                               memset(_privateData + len, c, count);
>                       return *this;
>               }
> 
>    I have already done this for my private fork, so I would be more than 
>    happy to provide a patch, unless someone comes up with a better idea...

Great. I've always tought we had to check for memory allocation failures, but I 
must confess I've never had the time/will to go on. So your work is VERY 
appreciated.

> 
> 3) MoveInto() in R5 works in a very peculiar way if it is asked to move more
>    characters than it contains: it just copies as many characters as 
>    requested (so it may segfault) but doesn't change the original string at
>    all!

Are we following that behaviour currently ?
Yes, I know I wrote that code, I just can't remember :))

>    Shouldn't we break compatibility with R5 and just move as many characters
>    as possible (without risking a crash, too)? 
>    I can't imagine any app is relying on this bug (surely not?), so I say
>    let's change the current
> 
[cut]
 
> Any comments/corrections are appreciated!

I'm all for it. If you provide a patch, I will be more than happy to apply it 
(if no one is faster, obviously).

P.S: I'm happy you're having a look at that code, since that way we can make 
the implementation better, and that's what opensource is about, isn't it ?
BTW, what about performance? I did some tests back then, and we were sometimes 
much faster, sometimes a bit slower than the R5 implementation... since you are 
actually using our BString implementation (and that's VERY good), I would like 
to know your experiences...

Bye!

Other related posts: