Re: StringNG merge?

On 18.12.2012 07:30, Kinkie wrote:
> On Mon, Nov 12, 2012 at 9:21 PM, Alex Rousskov
> <rousskov_at_measurement-factory.com> wrote:
>> On 11/03/2012 05:55 AM, Kinkie wrote:
>>
>>> Feature-branch is at lp:~kinkie/squid/stringng
>>
>>
<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());
>
> Er.. I don't think so: c_str() may cow(), which with the current code
> doesn't happen.
> It may be not really relevant as exceptions should not happen often.
> What is more valuable to you? Performance or code brevity?

The key detail is that c_str() *MAY* cow() whereas the above code
*ALWAYS* copies to a new exception buffer.
cow() is fine in this case since:

  1) it saves all the copies in those other cases where c_str() does not
cow().
  2) an exception is happening, chances are high that we are going to
abort and drop the data, or edit the SBuf anyway (ie fix header syntax
values).

>> src/ipc/TypedMsgHdr.cc appears to have changes unrelated to this
>> project.
>
> No changes appear appear with a file-based diff.. They were probably
> performed in trunk but trunk wasn't merged yet.
>
>> src/mem.cc has whitespace non-changes.
>
> Reverted to match trunk.
>
>> I assume you will drop StrList and friends from this submission so I
>> did
>> not continue to review them.
>
> Yes. The only question is whether to leave what was done so far in
> (#if 0-ed) or remove it.
>
>> I have not reviewed Tokenizer-related changes yet. I will wait for
>> all
>> other issues to settle first. Feel free to exclude Tokenizer from
>> the
>> proposed commit scope. It may be best to review Tokenizer when we
>> also
>> have some Squid code written or adjusted to use it.
>
> Sure, Tokenizer is not yet used. The same question arises as with
> StrList, whether I can leave it in (#if 0-ed) or I should cherrypick
> it out.

If it is easy to cherry-pick SBuf into trunk and fix Tokenizer later I
think we should do that approach simply to get the acceptable working
bits into trunk.

If that is not easy, IMHO we should audit and add both. They are both
needed in the end product anyway.
   OR, can Tokenizer work with SquidString / MemBuf / StringArea and go
in first?

Amos
Received on Mon Dec 17 2012 - 22:37:53 MST