[haiku-commits] Re: r40720 - in haiku/trunk: headers/os/support src/kits/support

On 2011-02-27 at 10:25:26 [+0100], jonas@xxxxxxxxxxx wrote:
> Author: kirilla
> Date: 2011-02-27 10:25:26 +0100 (Sun, 27 Feb 2011)
> New Revision: 40720
> Changeset: http://dev.haiku-os.org/changeset/40720
> 
> Modified:
>    haiku/trunk/headers/os/support/String.h
>    haiku/trunk/src/kits/support/String.cpp
> Log:
> Cleanup. Efficiency makerover.
[ ... ]
> 
> Modified: haiku/trunk/src/kits/support/String.cpp
> ===================================================================
> --- haiku/trunk/src/kits/support/String.cpp    2011-02-27 08:34:08 UTC (rev 
> 40719)
> +++ haiku/trunk/src/kits/support/String.cpp    2011-02-27 09:25:26 UTC (rev 
> 40720)
> @@ -425,20 +425,30 @@
>  
>  
>  BString&
> -BString::SetToArguments(const char *format, ...)
> +BString::SetToArguments(const char* format, ...)
>  {
> +    int32 bufferSize = 128;
> +    char buffer[bufferSize];
> +    
>      va_list arg;
>      va_start(arg, format);
> -    int32 bytes = vsnprintf(LockBuffer(0), 0, format, arg);
> +    int32 bytes = vsnprintf(buffer, bufferSize, format, arg);
>      va_end(arg);
> -    UnlockBuffer(0);

I know we're on the verge of nitpicking here, but don't just believe what 
Axel says ;-)

Using an on-stack buffer isn't so cheap after all, since that buffer has to 
be copied into the string when it happens to have been large enough. Instead, 
I would just use the string's current buffer:

        int32 bytes = vsnprintf(LockBuffer(0), Length() + 1, format, arg);

That has the additional advantage that the caller of this method can 
influence the initial size of the buffer beforehand.

> +
> +    if (bytes < 0) {
> +        return *this;
> +    }

no braces needed

> -    va_list arg2;
> -    va_start(arg2, format);
> -    bytes = vsnprintf(LockBuffer(bytes), bytes + 1, format, arg2);
> -    va_end(arg2);
> -    UnlockBuffer(bytes);
> -
> +    if (bytes < bufferSize) {
> +        SetTo(buffer);
> +    } else {

Again, no braces required, but I can see that it looks nicer because of the 
else part having them. Should we perhaps add that to our styleguide?

cheers,
        Oliver

Other related posts: