On Fri, Nov 4, 2016 at 6:12 PM, Alex Rousskov <[email protected]> wrote: > On 11/04/2016 01:12 AM, Kinkie wrote: >> On Thu, Nov 3, 2016 at 10:55 PM, Alex Rousskov >> <[email protected]> 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
sbufcontainerjoin-v4.patch
Description: Binary data
_______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
