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. _______________________________________________ squid-dev mailing list [email protected] http://lists.squid-cache.org/listinfo/squid-dev
