Re: [eigen] StdVector

[ Thread Index | Date Index | More Archives ]

Thank you!

I hope you feel as good about it as I do.


2009/4/21 Gael Guennebaud <gael.guennebaud@xxxxxxxxx>:
> OK, change done. (but I still have to update the documentation)
> Cheers,
> Gael.
> On Tue, Apr 21, 2009 at 10:27 PM, Benoit Jacob <jacob.benoit.1@xxxxxxxxx> wrote:
>> 2009/4/21 Markus Moll <markus.moll@xxxxxxxxxxxxxxxx>:
>>> Hi
>>> On Tuesday 21 April 2009 14:25:48 Gael Guennebaud wrote:
>>>> On Tue, Apr 21, 2009 at 2:08 PM, Markus Moll
>>>> > Sort of. The linker error is obviously gone, but the vector interface is
>>>> > still incorrect (it's at least missing a templated constructor from an
>>>> > iterator range, which makes existing code fail to compile), and to me it
>>>> > still feels very wrong to manipulate std::vector. I like the idea of
>>>> > forcing the user to use aligned_allocator _a lot_ better.
>>>> I fixed that issue 5 mn ago !
>>> Great.
>>> I have attached another patch that
>>> a) makes vector(size_type, const value_type& = value_type()) explicit and
>>> accept an additional const allocator_type&
>>> b) changes the __x, __a, __something names and _EIGEN_WORKAROUND_... that are
>>> reserved names to x, a, something and EIGEN_WORKAROUND...
>>> However, I haven't tested the patch so there could be some typos (I think
>>> not)...
>>>> Note that even if we enforce the user to use aligned_allocator, we
>>>> still have to reimplement the tricky resize functions that is the main
>>>> limitation because it depends on the platform
>>> Of course.
>>>> So basically, the only difference is that we would avoid the ugly
>>>> #define vector std_vector when including <vector>. But I don't think
>>>> that's a good reason to not keep the current solution which is simpler
>>>> to use.
>>>> > The question remains: is that worth it? And why only for std::vector?
>>>> > (leaving the resize problem aside)
>>>> because other stl types do not exhibit the "bug" of the resize
>>>> function. So, for other types just use aligned_allocator.
>>> That's exactly the point I was trying to make. With all other containers, the
>>> user has to "just use aligned_allocator", but for std::vector we try to avoid
>>> that requirement at all costs?
>> In my humble opinion... bingo!
>> Since we're requiring users to explicitly use aligned_allocator for
>> other containers, there's little benefit in avoiding that for
>> std::vector. The documentation doesn't get any simpler, and we break
>> the rule of "you're not annoyed by what you don't use" since our
>> std::vector specialization gets used for all types, not just Eigen
>> types.
>> So I'm very impressed by the latest StdVector but I can't help
>> thinking that this is going to remain a source of problems -- there's
>> already a good deal of MSVC-specific code. I think we'd be better off
>> switching right away to the idea that you (Gael) had proposed -- to
>> only specialize for Eigen::aligned_allocator. In my opinion that was
>> the right idea.
>> Cheers,
>> Benoit

Mail converted by MHonArc 2.6.19+