SmallVector::insert() fix

[David Blaikie]

On Mon, Jun 10, 2013 at 8:55 AM, Jean-Luc Duprat <jduprat at apple.com> wrote:
> I strongly agree that we've reached the point where SmallVector should just use std::vector once it outgrows the pre-allocated space.

Possibly an interesting hybrid state (between "just switch SmallVector
to std::vector entirely" and the current "fix SmallVector")

> Our current implementation is buggy, and will always be more work to maintain than reusing libc++.

Yep.

> We need consensus before I, or anyone else, goes off and writes a bunch of code.

Sure enough

> The current implementation is buggy and needs to be fixed.

Yep, one way or another.

> I sent out the initial email several weeks ago and people got bit by this issue since then.
> We could fix it and wait for C++11 adoption in the LLVM codebase, or move to one of Howard's proposal.

Yep.

The problem with Howard's proposals, it seems:
1) C++11 doesn't solve our "compiles as C++98" solution, and even if
we were to adopt C++11 in the LLVM codebase, it probably wouldn't
include C++ alias templates as MSVC doesn't support them so far as I
know
2) Switching to std::vector with an explicit allocator is verbose at
every call site
3) Switching to a type composed of std::vector with an explicit
allocator is verbose in implementation - but we don't have to
implement the entire std::vector API in the wrapper. Worst case we
just implement whatever SmallVector already has (ideally pruning
anything SmallVector has that std::vector doesn't, so that we can more
easily switch to (1) when that becomes an option)

Especially if you're proposing using std::vector once it outgrows the
pre-allocated space anyway, I'd be in favor of, at that point, writing
an allocator & just using std::vector always, with SmallVector being a
thin wrapper to keep client code not too verbose. Though I realize
writing an allocator is a bit of work compared to just having the
hand-hacked 'small' mode in SmallVector directly. But I think, as
Howard points out, it keeps us in well-tested code paths for more of
the implementation.

I'm not sure if you want take my word for it on that direction, but
that's my take on it.

> Avoiding to modify the API is probably a worthy goal, as there are a large number of users:
> $ grep -iIrn SmallVector lib/|wc -l
>     3919

We have tools (even sed could probably suffice here) that could easily
make such broad adjustments, so that shouldn't be much of a hindrance
to our choice of solution here.

>
>
> JL
>
>
> On Jun 7, 2013, at 13:59 , Michael Ilseman <milseman at apple.com> wrote:
>
>>
>> On Jun 7, 2013, at 10:23 AM, Howard Hinnant <hhinnant at apple.com> wrote:
>>
>>> On Jun 7, 2013, at 1:11 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>>
>>>>> Using C++11 template aliases you could even implement llvm::SmallVector<T, N> as a std::vector<T, llvm::stack_allocator<T, N>>
>>>>
>>>> This would be slightly problematic - is there anything we could do for
>>>> C++98 compatibility that would keep the terse naming we already have?
>>>
>>> It's ugly:
>>>
>>> template <class T, size_t N>
>>> struct SmallVector
>>> {
>>>   typedef std::vector<T, stack_allocator<T, N> > type;
>>> };
>>>
>>> SmallVector<T, N> has to become SmallVector<T, N>::type.  If T is a dependent template parameter, add a typename in front of SmallVector.
>>>
>>> The only other option I see (without template aliases) is:
>>>
>>> template <class T, size_t N>
>>> class SmallVector
>>> {
>>>   std::vector<T, stack_allocator<T, N> > data_;
>>> public:
>>>   // Replicate the interface here
>>> };
>>>
>>
>> The second option allows us to keep our asserts on access.
>>
>>> C++11 is worth migrating to. :-)
>>>
>>
>> Yup, I'm looking forward to that day.
>>
>>> Howard
>>>
>>> _______________________________________________
>>> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits