[squid-dev] [PATCH] Extend SBufContainerJoin to have prefix and suffix arguments

[Kinkie]

On Fri, Nov 4, 2016 at 6:12 PM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 11/04/2016 01:12 AM, Kinkie wrote:
>> On Thu, Nov 3, 2016 at 10:55 PM, Alex Rousskov
>> <rousskov at measurement-factory.com> wrote:
>>> On 11/03/2016 03:19 PM, Kinkie wrote:
>>>> On Tue, Nov 1, 2016 at 8:47 PM, Alex Rousskov wrote:
>>>>> On 11/01/2016 02:02 PM, Kinkie wrote:
>>>>>>   the attached patch extends SBufContainerJoin to have prefix and
>>>>>> suffix arguments.
>>>
>>>>> I recommend reworking this by adding a dest parameter:
>>>>> SBuf &ContainerToSBuf(SBuf &dest, ...);
>
>> attached v3 does this.
>
>>  SBuf
>> +JoinContainerIntoSBuf(SBuf &dest, ...
>
> Please return dest, not a copy of it. There is no reason to create and
> destroy a new SBuf object here IMO. SBuf creation and destruction are
> not as expensive as underlying storage copying, but they are still a
> measurable expense without justification AFAICT.

Doh, right. I had actually thought about this after sending the patch.

> There are certainly many cases where using references is a bad idea but
> returning a fed-by-reference "dest" is not one of them AFAICT. In fact,
> it is nearly the norm in "chainable" calls such as those that append
> something.

I agree that this case fits perfectly.

>> +/// variant of JoinContainerIntoSBuf not needing a SBuf lvalue
>
> I recommend a more informative description than retelling of the
> function declaration code. For example:
>
> /// convenience JoinContainerIntoSBuf() wrapper with an extra memory
> allocation

The extra memory allocation is a side effect; I agree it is a
convenience, but the convenience is in not needing a SBuf lvalue. This
makes it useable e.g. in ostreams

>
>> +    dest.reserveSpace(sz + prefix.length() + suffix.length());
>
> I would reorder the operands to make the code more self-documenting,
> especially when using an anonymous variable name like "sz":
>
>   dest.reserveSpace(prefix.length() + sz + suffix.length());

Ok. I'm also renaming sz for extra clarity.

>
>
>> I am not generally worried as it seems you are about copying SBufs as
>> long as the underlying storage is not reallocated needlessly.
>
> Unfortunately, all three patch versions reallocate storage needlessly
> (for various reasons). You can study the attached debugging from the
> attached patch to see what is actually going on in various
> JoinContainerIntoSBuf implementations. It may surprise and inspire you
> to make the code better. It did surprise and inspire me (but I wish I
> was not doing this legwork!).

Sorry about that causing extra work for you.

Updated patch attached. I'm struggling a bit with the convenience
wrapper documentation. Note that the wrapper cannot return a ref,
intentionally so. I hope RVO will optimize that cost away as much as
possible.

-- 
    Francesco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbufcontainerjoin-v4.patch
Type: application/octet-stream
Size: 5090 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161105/85fb1a46/attachment-0001.obj>