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

[Kinkie]

On Tue, Nov 1, 2016 at 8:47 PM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 11/01/2016 02:02 PM, Kinkie wrote:
>>   the attached patch extends SBufContainerJoin to have prefix and
>> suffix arguments. This can support a use-case which I found in the
>> current ACLRegexData work I'm following, where we need to transform
>> {"foo", "bar", "gazonk"}
>> into
>> (foo)|(bar)|(gazonk)
>>
>> It can be done with the current version of the SBufContainerJoin
>> function, but it requires one more copy.
>> There are probably further cases of this pattern elsewhere, all hand-rolled.
>
> I agree that the use case is legitimate, but the implementation does not
> solve the underlying cause -- extra copies.

It removes the need of one copy in a common use case, but it can do better.
It also has the drawback of iterating over a full container; reworking
to add a target SBuf and to use begin-end iterators instead of a
container.

> I recommend reworking this by adding a dest parameter:
>
> /// copies all container items (delimited by separator) into dest
> ...
> /// \returns dest
> SBuf &ContainerToSBuf(SBuf &dest, ...);
>
> and using that function to implement the SBufContainerJoin() or a
> similarly named convenience wrapper that does not require a dest
> parameter and that returns a copy of an internally used SBuf.

Can simply return the modified SBuf. All the convenience without the
bloat. Also taking the argument by copy; will cow if needed (passing
by reference makes for awkward client code as it'd have to be an
lvalue).
So the common pattern would be:

auto out = JoinContainerIntoSBuf(SBuf(), container.begin(),
container.end(), SBuf("separator"))

> In either case, please do not forget to document that the new prefix and
> suffix are always added, even if the container is empty.

Done.

>>      // sz can be zero in two cases: ... or all items are zero-length.
>
> FWIW, this is a lie. Zero-length items do not result in zero sz unless
> the separator itself is also surprisingly empty. Zero-length items is
> not a special case worth mentioning IMO. The zero-size check should be
> replaced with a direct (begin != end) check to avoid complexity and
> confusion -- it is only needed to avoid dereferencing begin() AFAICT.

Ok.

> HTH,

Sure does :)
Updated patch attached.


-- 
    Francesco
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sbufcontainerjoin-v2.patch
Type: application/octet-stream
Size: 3244 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20161103/e03563fd/attachment.obj>