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

[Ingo Weinhold <bonefish@xxxxxxxxxxxxxxx>]

On 2003-10-30 at 19:57:05 [+0100], you wrote:
> Oliver Tappe wrote:
> 
> > 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.

Reading the not exactly detailed BeBook documentation -- `LockBuffer() 
returns a pointer to the object's string...' -- as a user I would actually 
expect that the pointer returned by LockBuffer() contains the C string 
represented by the object as a prefix. So, in case of the empty string its 
first character should indeed be a '\0', IMHO.

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

I'm not so sure, if that's a good idea. This will cause the implementation to 
doubly touch the new space in the cases where _GrowBy() and _OpenAtBy() are 
used, thus decreasing performance. In case of LockBuffer() it will even be 
worse, if the allocated size is far greater than the actually used one.

BTW, it might be a good idea to not allocate memory, when _GrowBy() is asked 
to create an empty string, but rather free the previously allocated memory 
(if any) and set _privateData to NULL.

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

Yep, sounds good.

CU, Ingo

• References:
  • [interfacekit] BString patch suggestion and some questions, too
    • From: Oliver Tappe
  • [interfacekit] Re: BString patch suggestion and some questions, too
    • From: Erik Jaesler

Other related posts:

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

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

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

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

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