Re: [PATCH] SBufList

On 12/02/2013 02:03 PM, Francesco Chemolli wrote:

> it shouldn't, as each append
> operation is at the tail of the backing MemBlob's used portion.

Yes, your "append into the unused MemBlob space if possible"
optimization is working well here. The accumulate-based solution still
feels rather fragile to me, but I do not have enough reasons to push for
a redesign.

> Please let me know if there's any other questions, or if it's OK to merge now :)

> + inline bool operator() (const SBuf & checking) { return 0 == checking.compare(reference_,sensitivity_); }
> + inline bool operator() (const SBuf & checking) { return checking.startsWith(prefix_,sensitive); }

The "inline" keyword is not needed (and is used inconsistently
throughout the patch).

> + inline bool operator() (const SBuf & checking) { return 0 == checking.compare(reference_,sensitivity_); }

This still uses unnatural order of operands.

> + SBufCaseSensitive sensitivity_;

but

> + SBufCaseSensitive sensitive;

The above members are named inconsistently. Your call, but I prefer the
top version.

> +#include <algorithm>

That header does not define std::accumulate() on some platforms. Use
<numeric>.

Finally, AFAICT, v3 of the patch appears to be missing Makefile.am
changes necessary for "make check" to engage the new test cases: There
is not testSBufList in src/Makefile.am.

It looks like you smuggled in SBufList.{cc,h} lines into
src/icmp/Makefile.am earlier (trunk r13033 and merged branch
r12745.1.23), before those two files were actually added to trunk
sources, so "make" might work (it did not for me, but there was an
unrelated <numeric> problem mentioned above, and my tree is too dirty to
test reliably).

HTH,

Alex.
Received on Mon Dec 02 2013 - 22:55:08 MST