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

[Ryan Flynn]

On Sun, Jul 12, 2009 at 3:43 PM, Daniel Dunbar<daniel at zuster.org> wrote:
> 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
>>
>>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>

Daniel,

Good points. Better to keep things simple.

Ryan