Re: [PATCH] SBufList

On Fri, Nov 29, 2013 at 2:26 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 11/26/2013 02:28 AM, Kinkie wrote:
>
>> +bool isMember(const SBufList &, const SBuf &, SBufCaseSensitive isCaseSensitive = caseSensitive);
>
> Please capitalize global names, such as isMember().

Ok

>> +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.
Do you agree these two small auxiliary classes belong to SBuf.h or do
you think they should go elsewhere?

>> +SBuf
>> +SBufListJoin(const SBufList &list, const SBuf &separator)
>> +{
>> + if (list.size() == 0)
>> + return SBuf("");
>> + if (list.size() == 1)
>> + return list.front();
>
> This may be very ineffient for large lists and is probably unnecessary.
> Avoid list.size() calls if it all possible. std::list::size() is O(n),
> not O(1) in some older GCC implementations (at least). For one heated
> discussion, see http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49561
>
>
>
>> + 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. If so,
would SBuf.h be a good place?

>>>> attached is v1 of SBufList. Its purpose is to be the eventual heir
>>>> to wordlist.
>
>
> It may be useful to take a step back and decide whether we need a
> one-size-fits all wordlist or many SBuf containers, with different
> storage/access trade-offs. I am pretty sure it is the latter. The above
> suggestions are meant to make your work usable with all containers (and
> minimize code duplication).

I'd like to avoid scope creep.
This code is meant to supersede wordlist and bring the huge amount of
code that uses it into the c++ world (and into the world of managed
memory), and it can certainly be a step towards that end goal .
SBuf should be reasonably container-friendly and can certainly be made
even more so, but I'd prefer to address things as the need arises
instead of overdesigning them.

I'm attaching the revised patch.

-- 
    /kinkie