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

Oliver Tappe wrote:

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.

Rather than removing the strcmp() test, I would much rather see _GrowBy() initialize new space to '\0'. Since a default-constructed BString is otherwise ready for use, it seems inconsistent that LockBuffer() can return something that isn't. As it is, if I have

        BString myString("test");
        myString.LockBuffer(10);

_GrowBy() is going to leave me with "test\0xxxxx\0" (where 'x' is random cruft), which is not very hygenic. ;) So making the suggested change will not only give predictable results for default-constructed BStrings, but just be generally more clean. Depending on what the use cases are, it might be a good idea to apply this to _OpenAtBy() as well.

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

According to [1], realloc's failure mode is such that "if the space cannot be allocated, the object shall remain unchanged." It seems more correct that BString behave in a similar fashion, especially as no direct indication of an error is made. Of course, since the app would be in a severe low memory situation, it will probably crash relatively quickly anyway, so it's almost an aesthetic argument. ;) In order to ease internal handling of failed allocations, however, _GrowBy(), ShrinkAtBy() and _OpenAtBy() could still return NULL.

e

[1] http://www.opengroup.org/onlinepubs/007904975/functions/realloc.html


Other related posts: