Re: [PATCH] SBufList

On Mon, Dec 2, 2013 at 1:36 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 12/01/2013 02:41 PM, Kinkie wrote:
>> On Fri, Nov 29, 2013 at 2:26 AM, Alex Rousskov wrote:
>>> On 11/26/2013 02:28 AM, Kinkie wrote:
>>>
>>>> +bool isMember(const SBufList &, const SBuf &, SBufCaseSensitive isCaseSensitive = caseSensitive);
>
> The last parameter can be const as well.

Done.

>>>> +bool
>>>> +isMember(const SBufList & sl, const SBuf &S, SBufCaseSensitive case_sensitive)
>>>> +{
>>>> + for (SBufList::const_iterator i = sl.begin(); i != sl.end(); ++i)
>>>> + if (i->compare(S,case_sensitive) == 0)
>>>> + return true;
>>>> + return false;
>>>> +}
>>>
>>> STL already provides std::find() and std::find_if() that are more
>>> flexible/universal and sometimes faster than isMember(). I suggest that
>>> you provide a predicate to be used for case-specific comparisons of
>>> buffers instead of isMember(). Such a predicate would be usable with any
>>> SBuf container, not just SBufList.
>>
>> I suspect that these better belong to SBuf.cc than to SBufList.cc then.
>> Done so, and with perfect feature creep made it parametric (case
>> sensitive/insensitive) and built one also for prefix match.
>
> This is not feature creep IMO. This is simply the Right Way to support
> SBuf search in STL containers.

Ok. I'm taking this chance to learn :)

>> +class SBufComparePredicate {
>
> This only compares for equality. I suggest naming it SBufEqual.

Ok

>> +class SBufPrefixComparePredicate {
>
> This only compare for prefix-equality. I suggest naming it SBufStartsWith.
>
> If you think an explicit "Predicate" suffix is needed for some reason,
> add that or just "P". Personally, I do not see a need for either.

Ok.

> Please document each predicate class. Currently only the first is
> documented. A one-line description is enough for each IMO. For example:
>
> /// SBuf equality predicate for STL algorithms and such
>
> /// SBuf "starts with" predicate for STL algorithms and such

Excellent. Done.

>> + SBufPrefixComparePredicate(); //no default constructor
>> + SBufComparePredicate(); //no default constructor
>
> Why declare one if it should not be available? The compiler will not
> generate one because you already have a non-default constructor.

Ok

>> +class SBufPrefixComparePredicate {
>> +public:
>> + explicit SBufPrefixComparePredicate(const SBuf s, SBufCaseSensitive caseSensitive = caseSensitive) :
>> + compareWith(s), sensitive(caseSensitive) {}
>> + inline bool operator() (const SBuf & checking) { return 0 == checking.startsWith(compareWith,sensitive); }
>> +private:
>> + SBuf compareWith;
>
> I would rename compareWith member in this class to "prefix". This will
> help document what the constructor parameter is meant to be.

renamed SBufEqual's parameter to "reference" and this to "prefix".
Used underscors suffix for private data members.

>> + explicit SBufComparePredicate(const SBuf s, SBufCaseSensitive caseSensitive = caseSensitive) :
>> + explicit SBufPrefixComparePredicate(const SBuf s, SBufCaseSensitive caseSensitive = caseSensitive) :
>
> * "s" can be a reference to avoid an extra refcounting overhead.

Yes.

> * caseSensitive can be const

Ok.

> * please use a unique name for the formal parameter; I suggest "sensitivity"
>
>
>> + SBufCaseSensitive sensitive;
>
>> + SBufCaseSensitive sensitive;
>
> I suggest sensitivity_ for both names.

What about caseSensitivity for the parameter, and sensitivity_ for the
private data member?
I notice only now the overlap between the parameter name and the value.

> BTW, the SBufCaseSensitive type should be similarly renamed as well IMO.
> Why is it even declared outside of the SBuf class?

Any suggestion is welcome, but I'd do that as a separate commit; for
now please let's leave it as is, I'm adding a TODO.

>> + inline bool operator() (const SBuf & checking) { return 0 == checking.compare(compareWith,sensitive); }
>
>> + return sl.end() != std::find_if(sl.begin(), sl.end(),
>> + SBufComparePredicate(S,case_sensitive));
>
>
> .arguments == of order natural a use Please

Done so I have.

>> + inline bool operator() (const SBuf & checking) { return 0 == checking.startsWith(compareWith,sensitive); }
>
> startsWith() returns bool. No need to compare it with anything.

Yes. That was broken. Sorry.

>> Do you agree these two small auxiliary classes belong to SBuf.h or do
>> you think they should go elsewhere?
>
> SBuf.h is fine IMO.

Ok.

>> + //
>> + SBufList::const_iterator i = list.begin();
>> + rv.append(*i);
>> + ++i;
>> + for (; i!= list.end(); ++i) {
>> + rv.append(separator);
>> + rv.append(*i);
>> + }
>
> Consider using std::accumulate with an appropriate "operator" class
> instead of the above. See a sketch below.
>
> There is also an empty comment at the start of that block.

Doh. Added comment.

>> + sz += separator.length() * list.size();
>
> Not a big deal, but this is wrong for single-item lists that do not use
> a separator.

It's only an optimization, worst case the resulting SBuf will have one
separator.length() extra backing storage. As you already mentioned,
the elephant there is list.size() anyway..

>> + rv.rawSpace(sz);
>
> Please use reserveSpace() if you do not need to access the reserved
> space immediately.

Ok.

>>>> + SBufList::const_iterator i;
>>>> + // calculate the total size of the return value
>>>> + SBuf::size_type sz = 0;
>>>> + for (i = list.begin(); i != list.end(); ++i)
>>>> + sz += i->length();
>>>> + sz += separator.length()*list.size();
>>>> +
>>>> + SBuf rv;
>>>> + rv.rawSpace(sz);
>>>> +
>>>> + i = list.begin();
>>>> + rv.append(*i);
>>>> + ++i;
>>>> + for (; i!= list.end(); ++i) {
>>>> + rv.append(separator);
>>>> + rv.append(*i);
>>>> + }
>>>> + return rv;
>>>> +}
>>>
>>>
>>> Consider using std::accumulate with an appropriate operator instead of
>>> the above. Providing such an operator would allow folks to "join" SBufs
>>> stored in any SBUf container, not just SBufList.
>>
>> Size precalculation is an optimization. I've made use of
>> std:accumulate via SBufListAccumulateSizes, but I'm not sure it'd be
>> worthwile to make that available throughout the code base.
>
> My point was that joining SBuf containers is useful for all container
> types, but your code only works for std::lists. I think you can provide
> the same functionality, with the same level of optimization, by making
> the code generic. Here is a sketch:
>
> {
> SBuf::size_type mergedSize = 0;
> std::accumulate(..., mergedSize, AddSize(separator));
>
> SBuf merged;
> merged.reserveSpace(mergedSize);
> std::accumulate(..., merged, AddContent(separator));
>
> return merged;
> }
>
> The missing "..." parts just contain items.begin(), items.end(), where
> bufs is the container.

Ok.
Let's try to sketch the grand plan, shall we?
What about using a SBufAlgs.h which contains

SBufEqual
SBufStartsWith
SBufAddLength(separator=SBuf())

template<class container> SBufContainerJoin<container>(...)

plus a specialized SBufListJoin which is simply an instantiation of
SBufContainerJoin<SBufList>?

This should cover it all.

Thanks,
    /kinkie
Received on Mon Dec 02 2013 - 12:23:59 MST