Re: [PATCH] SBufList

On 12/02/2013 11:34 AM, Kinkie wrote:

> + SBuf operator() (SBuf accumulated, const SBuf &item) {
> + if (first) {
> + first = false;
> + accumulated.append(item);
> + return accumulated;
> + }
> + accumulated.append(separator_).append(item);
> + return accumulated;
> + }

> + SBuf rv;
> + rv.reserveSpace(sz);
> + rv = std::accumulate(items.begin(), items.end(), rv, SBufAddContent(separator));

The above combination feels wrong. Would not the above result in the
pre-allocated accumulation buffer (rv) disappearing after the first
operation, and every consecutive operation allocating a new, bigger buffer?

If my suspicion is correct, then I think the solution is to move away
from std::accumulate for content accumulation. That algorithm is from
<numerics> so perhaps it was never meant for this kind of in-place
buffering optimization. We can use std::for_each instead (placing the
accumulation buffer inside the AddContent function class, just like we
placed the separator) but perhaps you can find a more appropriate
algorithm at http://www.cplusplus.com/reference/algorithm/

The first std::accumulate use (to calculate the size) seems OK.

And yes, I know I used std::accumulate twice in my sketch. This just
proves that STL is not my strong suit (and illustrates why I hate
sketching things and effectively ending up taking responsibility for any
problems).

>>> plus a specialized SBufListJoin which is simply an instantiation of
>>> SBufContainerJoin<SBufList>?
>>
>> What will that specialization do differently? I do not think we need it
>> but perhaps I am missing something.
>
> As above: just convenience,

Calling SBufContainerJoin(l, s) is as convenient as calling
SBufListJoin(l, s), right? I do not see the added convenience here.
IsMember() adds a little bit of convenience, but SBufListJoin() does not
seem to do that.

> +/** convert a wordlist to a SBufList
> + */

Using /// would save you one line and would prevent the comment from
looking so awkward :-)

> +SBufList wordlistToSbufList(wordlist *);

This one can be called ToSbufList(). The compiler will know you are
converting wordlist based on the parameter type. There may be more
functions like that in the future...

Cheers,

Alex.
Received on Mon Dec 02 2013 - 20:37:14 MST