Re: [Merge] lp:~squid/squid/sbuflist-merge into lp:squid

On 14 Dec 2013, at 12:14, Amos Jeffries <squid3_at_treenet.co.nz> wrote:

> On 14/12/2013 11:17 p.m., Francesco Chemolli wrote:
>> Francesco Chemolli has proposed merging lp:~squid/squid/sbuflist-merge into lp:squid.
>>
>> Requested reviews:
>> squid (squid)
>>
>> For more details, see:
>> https://code.launchpad.net/~squid/squid/sbuflist-merge/+merge/199024
>>
>
> in src/SBufAlgos.h:
> * SBufAddLength::separator_len does not meet camelCase_ private member
> naming guideline.

fixed.

> * the if(sz == 0) and comment do not match up.
> - I suspect it should probably be a check on items.size() to match the
> comment and the whole if() be done before the sz accumulate is attempted.

Clarifying the comment.
sz is zero if one of two cases happens: if items is empty, or if all items are zero-length.
This check will protect against the first case in a more efficient way than using items.size() (Alex mentioned items.size() being O(N) on some implementations) and optimize the second case.
I've updated the comment to reflect this fact.

> * #includes block is inconsistent with rest of Squid which places empty
> line between "local" and <system> include statements.

Fixed.

> in src/SBufList.cc:
>
> * the #include SBufAlgos.h should be sorted before SBufList.h

Ok

> * #include wordlist.h seems not needed. Yes?

It isn't.
Removing.
Scheduling a full matrix build. If it succeeds, I'll merge in a few hours.

Thanks for taking the time, sorry for the spamming.

  Kinkie
Received on Sat Dec 14 2013 - 15:49:49 MST