[interfacekit] BString patch suggestion and some questions, too

Hi everyone,

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

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.

2) memory-allocations aren't checked for failure at all, so BString will   
   just crash and burn in case of memory-shortage.
   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...

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!
   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

                BString&
                BString::MoveInto(BString &into, int32 from, int32 length)
                {
                        if (&into == this)
                                return *this;
                        int32 len = Length() - from;
                        len = min_c(len, length);
                        into.SetTo(String() + from, length);
                        if (from + length <= Length())
                                _privateData = _ShrinkAtBy(from, len);
                        return *this;
                } 

        to

                BString&
                BString::MoveInto(BString &into, int32 from, int32 length)
                {
                        if (&into == this)
                                return *this;
                        int32 len = Length() - from;
                        len = min_c(len, length);
                        into.SetTo(String() + from, len);
                        _ShrinkAtBy(from, len);
                        return *this;
                } 

Any comments/corrections are appreciated!

cheers,
        Oliver

Other related posts: