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

[Alex Rousskov]

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.

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.

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


>      // 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.


HTH,

Alex.