[squid-dev] [PATCH] SBuf API improvements

[Kinkie]

On Mon, Dec 28, 2015 at 10:27 PM, Alex Rousskov
<rousskov at measurement-factory.com> wrote:
> On 12/28/2015 06:41 AM, Kinkie wrote:
>
>>   the attached patch is a proposal for a couple of small extensions to
>> SBuf, which are meant to make it more API-compatible with std::string.
>
> ... as well as increase the probability of callers using the wrong SBuf
> API as discussed below.
>
>
>> - it adds a "fill" operator: append(size_type n, char c)
>> - it aliases append(char) to push_back(char)
>
> If this change goes forward, I recommend removing our custom
> append(char) because it conflicts with the other "standard" append,
> increasing API confusion:

Sure, can do.
It will probably look funny in chaining code, but that's not different
from std::string, e.g.:
SBuf s;
s.append("string").push_back('c').append("another string");

>
>   string& append(const char* s);
>
> The old append(char) calling code would need to be adjusted to call
> push_back(), of course.

Of course.

>
>> - it aliases reserveCapacity to reserve
>
> The standard reserve() has a default argument value of zero. Yours does
> not. If you want compatibility, you have to provide that default IMO.

Ok, done. If reserve() is called with no arguments, il'll be a no-op,
but that's consistent with the documentaiton I found.

>> It is debateable whether reserveCapacity should be just renamed
>> instead of aliased; feedback is welcome.
>
> I do not have a strong opinion on this. reserveCapacity() is not called
> reserve() today because we also have reserveSpace() and knowing that
> there are two different methods is important for some callers. Once you
> add plain reserve(), the probability that a caller will notice
> reserveSpace() that the caller needs will go down.

We could leave both and mark in the documentation that it's meant for
API compatibility and should not be used by code calling SBuf. what do
you think?

>> The objective of this change will be clear in the followup rfc3986
>> patch, which is templatized in order to be available both to helpers
>> who use std::string and to squid using SBuf.
>
> SBuf is already suffering from being too many things to too many
> different callers. I suspect that requiring API compatibility with
> std::string [in certain poorly defined areas] will make the situation
> worse, but I cannot prove that. Same for writing templated code that can
> work with both std::string and SBuf.
>
> If helpers really need SBuf, give them SBuf. If helpers want to use
> std::string, they already can. Assuring that one can be substituted with
> another feels like a false goal that would bring more trouble in the
> long run than it would help in the short term. Again, I cannot prove
> that and will not veto these changes.

helpers do not need SBuf, in fact they don't want it, they need
percent-encoding and a c++ string management system.
 The main benefit of SBuf over std::string is its integration with
support infrastructure in squid proper (mempools and cachemgr), which
is definitely too much to add to helpers as well. We'd end up filling
them with stubs which would make maintaining the build system
complicated - see how much effort it is to do so for unit tests.

So IMO helpers need std::string, and templated code is the best
solution I could come up with to avoid having to maintain three
different implementations of percent-encoding. Suggestions on
alternate paths to obtain the same objective are of course welcome.

As you don't veto the code, but neither approve it; are there others
willing to add their thoughts?

Thanks
(I've implemented some of the changes but won't push a patch until
open points have been closed. Thanks)

-- 
    Francesco