Re: [PATCH] refactor Vector

On 5/02/2014 8:57 a.m., Kinkie wrote:
> On Sun, Feb 2, 2014 at 10:42 PM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
>> On 2014-02-03 08:06, Kinkie wrote:
>>>
>>> Hi,
>>> the attached patch (merge from lp:~squid/squid/vector-refactor) is
>>> an attempt to refactor Vector and its clients so that:
>>> - clients of Vector don't break layering
>>> - the Vector API more closely matches std::vector
>>>
>>> The eventual aim is to replace Vector with std::vector, only if some
>>> more accurate measurements (in the lp:~squid/squid/vector-to-stdvector
>>> branch) show that this wouldn't cause performance degradations.
>>>
>>> Don't be fooled by the size of the patch; it is big because there's
>>> lots of context.
>>>
>>> Thanks
>>
>>
>> in src/HttpHdrRange.cc
>>
>> * This pattern happens several times throughout this patch:
>> - while (!specs.empty())
>> - delete specs.pop_back();
>> + while (!specs.empty()) {
>> + delete specs.back();
>> + specs.pop_back();
>> + }
>>
>> ** std::vector::pop_back() is documented as erasing the element.
>> Squid::Vector does not.
>> - under correct usage of std::vector the loop and delete X.back() would
>> seem to be irrelevant.
>> ** the pattern with loop seems to be equivalent to specs.clear() but far
>> less efficient. Efficiency is saved here only by Squid::Vector not
>> reallocating the items array in pop_back().
>
> Both left in after Alex' comments.

Okay.

>
>> in src/HttpHeader.cc:
>> * please replace C-style casts with C++ casts in lines changed. static_cast
>> would be appropriate in several places in this patch, if needed at all.
>
> Done.
>
>> in src/base/Vector.h
>> * still need to remove operator +=. There are likely more conversions to
>> push_back() hiding as a result.
>
> Done. There were two.
>
> v2 attached.
>

Okay. Looks good.

+1.

Amos
Received on Thu Feb 06 2014 - 12:12:49 MST