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

[Alex Rousskov]

On 11/05/2016 02:57 PM, Kinkie wrote:
> On Fri, Nov 4, 2016 at 6:12 PM, Alex Rousskov wrote:
>> On 11/04/2016 01:12 AM, Kinkie wrote:
>>> +/// 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; 

Some side effects, like this one, are very important to document,
_especially_ because they are invisible to the caller. If I am looking
for a convenience function to call, I do want to know whether that
convenience comes at a price.


> the convenience is in not needing a SBuf lvalue.

Yes.


> This makes it useable e.g. in ostreams

Both functions are usable with ostream, but the convenience wrapper is
more convenient to use, naturally. There is nothing wrong with that! You
just need to warn folks that they are probably paying one memory
allocation/destruction for that convenience. In many cases, that price
is perfectly reasonable, of course. We just need to disclose it.


> Note that the wrapper cannot return a ref, intentionally so.

Yes, of course. As far as the wrapper is concerned, it is only a
question of disclosing that the wrapper is a costly function. The cost
does not come from the return type. It comes from the lack of a
pre-allocated caller's buffer to accumulate the results in.


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

> Updated patch attached. 

> +    dest.reserveSpace(prefix.length() + totalContainerSize + suffix.length());

Please note that v4 still allocates memory according to my last
experiment. See JoinContainerIntoSBuf3() which mimics your patch v4. You
may claim that the unnecessary allocation is not the fault of this
patch, and you could be right, but I was hoping this observation will
inspire you to change

* either your patch (mimicking my JoinContainerIntoSBuf4() which does
not result in extra allocation) reserveSpace()

* or reserveSpace() (after making sure that all callers are going to be
OK with that change).

The latter would be better if it is possible. If it is not possible,
then we may need to change reserveSpace() documentation so that folks do
not use that method like you did.


HTH,

Alex.