Re: StringNG merge?

On Mon, Dec 17, 2012 at 11:37 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> 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).

Whoops, I just realized by looking the code that this whole discussion
is over nothing: the SBuf we're talking about is the explanatory
message, which is created in the same method. So there is only one
reference by definition, no copying.

message = xstrndup(explanatoryText.c_str(),explanatoryText.length());

and that's it.

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

Merge will be by patch, not bzr merge. Which means that it should be doable.

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

I'd recommend against adapting tokenizer to a different backing store;
it'd mean double the work for a stopgap measure.

-- 
    /kinkie