Re: StringNG merge?

On 11/12/2012 03:40 PM, Amos Jeffries wrote:
> On 13.11.2012 09:21, Alex Rousskov wrote:
>> On 11/03/2012 05:55 AM, Kinkie wrote:
>>
>>> Feature-branch is at lp:~kinkie/squid/stringng
>>
>>
>> Found one or two more bugs and a few nits:
>>
> <snip>
>>> + message = static_cast<char*>(xmalloc(buf.length()+1));
>>> + buf.copy(message,buf.length());
>>> + message[buf.length()]='\0';
>>
>> This code can be simplified by writing:
>>
>> message = xstrdup(buf.c_str());

Actually, this should be "b.cstr()" not "buf.c_str()". Too many similar
b-names: I think the original code (quoted below) should also say "b"
instead of "buf" in a few places!.. Kinkie, please review that code
carefully even if you do not simplify. Using a more appropriate name for
"b" may help it make less confusing (e.g., "msgBuf").

> Two reasons, by importance:
>
> 1) using buf.c_str() will require terminating buf.content(), possibly
> with a termination cow() which can be avoided by using this inline copy

In this case, the message-building buffer is a temporary local variable
not shared with any other code. There will be no copying on write, and
the size can be reserved appropriately to avoid extra allocations. The
current code does not reserve message-building buffer capacity at all,
so we are not making things worse by simplifying.

> 2) one of the goals of SBuf rollout is to erase xstrdup() and its
> null-terminated requirement from Squid.

In this particular case, xmalloc() followed by manual null-termination
is even worse than a simple xstrdup()!

HTH,

Alex.

> +OutOfBoundsException::OutOfBoundsException(const SBuf &buf,
> + SBuf::size_type &pos,
> + const char *aFileName,int aLineNo)
> + :TextException(NULL,aFileName,aLineNo)
> +{
> + _buf=buf;
> + _pos=pos;
> + SBuf b("OutOfBoundsException");
> + if (aLineNo!=-1)
> + b.appendf(" at line %d",aLineNo);
> + if (aFileName!=0)
> + b.appendf(" in file %s",aFileName);
> + b.appendf(" while accessing position %d out of %d",pos,buf.length());
> + message = static_cast<char*>(xmalloc(buf.length()+1));
> + buf.copy(message,buf.length());
> + message[buf.length()]='\0';
> +}
Received on Mon Nov 12 2012 - 23:50:55 MST