Re: [PATCH] refactor Vector

On Fri, Feb 7, 2014 at 6:32 AM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 02/04/2014 12:57 PM, 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.
>
>> v2 attached.
>
>> +Vector<E>::at(unsigned i)
>> {
>> assert (size() > i);
>> return items[i];
>> }
>>
>> template<class E>
>> const E &
>> -Vector<E>::operator [] (unsigned i) const
>> +Vector<E>::at(unsigned i) const
>> {
>> assert (size() > i);
>> return items[i];
>> }
>
> Ideally, at() methods should be implemented using [] operators instead
> of direct access to "items", but that is very minor.

Done.

>> - ErrorDynamicPageInfo *info = ErrorDynamicPages.items[i - ERR_MAX];
>> + ErrorDynamicPageInfo *info = ErrorDynamicPages[i - ERR_MAX];
>
> This non-performance-critical line surrounded by relatively complex
> index logic should use an at() method instead. Again a minor thing.

Done.

>> - theClient->start (tag + 1, (const char **)attributes.items, attributes.size() >> 1);
>> + theClient->start (tag + 1, const_cast<const char **>(attributes.data()), attributes.size() >> 1);
>
> The kind of cast appears to be wrong here: Const_cast<> is normally used
> to remove const, not add it. It is not used to change the type.
>
> Also, .data() is a C++11 method. Should we avoid those for now? To be
> compatible with older STLs, you may use the address of vector[0] instead
> AFAICT.

Yes, data() is a c++11 meethod; I've implemented like this because
it's the least change against the current code.
In fact, I hit a snag with that call: the char ** argument is passed
quite deep into the ESI code; from what I understand it is used to
represent a list of <argument,value> pairs. Changing that to vectors
or maps is nontrivial.
And I don't see how to reliably get to vector[0] in pre-11 STL. Do you
have any clue?

Thanks

> All of the above can be addressed during commit IMO.

-- 
    Francesco