Re: StringNG merge?

On Tue, Nov 13, 2012 at 12:50 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> 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").

Renamed respectively throwingBuf and explanatoryText, and yes, you're
right. It was copying the wrong text.

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

It may affect reservations on the underlying storage, from what I can see.

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

If only xstrdup could take an optional length parameter...
Would you agree to extending it (or defining an xstrndup which does) ?

Thanks

-- 
    /kinkie