Re: [PATCH] SBufList

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().

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

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

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

HTH,

Alex.
Received on Fri Nov 29 2013 - 01:26:53 MST