Re: [PATCH] SBufList

On 02 Dec 2013, at 23:54, Alex Rousskov <rousskov_at_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
Received on Tue Dec 03 2013 - 07:49:51 MST