On 02 Dec 2013, at 23:54, Alex Rousskov <rouss...@measurement-factory.com> wrote:
> 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. That optimisation is one of the three main reasons why SBuf is expected to work better for us over std::string (the other two are effective sub stringing for the Tokenizer and MemPools integration). It's no big deal, I can revert to manually iterating or using for_each or similar. >> 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). Removed. > >> + inline bool operator() (const SBuf & checking) { return 0 == >> checking.compare(reference_,sensitivity_); } > > This still uses unnatural order of operands. Changed >> + SBufCaseSensitive sensitivity_; > > but > >> + SBufCaseSensitive sensitive; > > The above members are named inconsistently. Your call, but I prefer the > top version. Yes, corrected. >> +#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. Sorry, my fault for not clarifying it. As this patch is a cherrypick of lp:~squid/squid/stringng, I'm not extracting the Makefile.am changes as it's too time-consuming. These changes are present in the branch Makefile.am and will be included at the final merge time, but are not really significant for review, are they? > 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). Oops, you're right, I'm removing them from trunk. Kinkie