Re: [PATCH] refactor Vector

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.

> - 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.

> - 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.

All of the above can be addressed during commit IMO.

Thank you,

Alex.
Received on Fri Feb 07 2014 - 05:32:27 MST