[llvm-commits] patch: SmallVector fix outstanding issue via unit test

[Daniel Dunbar]

Hi Ryan,

Thanks for the patch. It looks fine to me, although I am ambivalent
about adding 'const' to the method parameters -- conceptually those
parameters are not 'const' in the sense that the routine could modify
the array through them and still honor its contract, it just happens
to not be implemented that way.

As for the main change to resize(), the current implementation matches
std::vector::resize (which accepts an argument for the value to insert
when padding). Since this seems to be only a slight performance tweak
(and it the current implementation could be better, depending on the
work done in the default constructor vs the copy constructor), it
seems (marginally) better to match the std::vector semantics. I will
tweak the unit test to explain where the extra copy comes from.

I'm open to other opinions though...

 - Daniel

On Wed, Jul 8, 2009 at 5:36 PM, Ryan Flynn<parseerror at gmail.com> wrote:
> the copy constructor for SmallVector.resize(N) was being called N+1 times.
> the unit test was modified to account for this 'unexplained' behavior.
>
> resize() is creating a temporary object of type T for the sole purpose
> of using it as a copy constructor for elements via construct_range()
>
> i created an overloaded version of construct_range() omitting the T obj
> parameter and call T's default constructor instead, a more efficient
> equivalent to copy-constructing an object which is itself default-
> constructed.
>
> also removed temporary variable 'E' from SmallVector operator==, it is
> equivalent to member 'End' which already exists.
>
> also added 'const' to SmallVector's {construct,destruct}_range()
> method parameters where possible.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>