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

Oliver Tappe <openbeos@xxxxxxxxxxxxxxx> wrote:
> 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.

Indeed.

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

Although I think the BString class should not crash under any 
circumstance, the class is not designed to propagate any errors back.
I am not sure if it's a good idea to just delete the current string in 
case it couldn't be enlarged, I am also not sure to deviate from the 
expected behaviour without having the option to check a return value.
But anyway, I think it's better than have it crashing. We could throw 
an exception in the future as well.

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

Yes, that should be done in any case!
But is:
        if (&into == this)
                return *this;
correct? Shouldn't it just contain from + length after this method has 
been called?

Bye,
   Axel.

Other related posts: