libc++: First cut at <dynarray>

[Howard Hinnant]

On Sep 10, 2013, at 12:38 PM, Marshall Clow <mclow.lists at gmail.com> wrote:

> On Sep 9, 2013, at 4:48 PM, Howard Hinnant <howard.hinnant at gmail.com> wrote:
> 
>> Thanks Marshall.  I've read Richard's excellent comments, and these are in addition to his.
>> 
>> ...
>> 
>> Marshall and I have already discussed this, but for the benefit of others:  I don't think dynarray should ever use the heap, and I don't want to commit even temporarily an implementation that does.  We need the stack support from the compiler prior to committing.  And that subsequently will probably require a __has_feature for the stack allocation support.
> 
> I disagree.  ;-)
> 
> I think that dynarray should use the stack whenever possible. 
> But things like "new dynarray<int> (10)" clearly can't use the stack for storage. [ dynarray as a member variable is another example. ]
> 
> I agree that this will require compiler support, and a built-in function and a __has_feature, but unless the compiler can see the entire lifetime of the dynarray, the storage will have to go on the heap.
> 
> That being said, I think that most of the common use cases of dynarray will fall into the "be able to put on the stack" category.
> 
> 
>> On Sep 9, 2013, at 12:06 PM, Marshall Clow <mclow.lists at gmail.com> wrote:
>> 
>>> <dynarray> See http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3662.html
>>> 
>>> Open Issues:
>>> 1) This includes <tuple> because that's where __uses_alloc_ctor lives. I'm thinking that __uses_alloc_ctor should be hoisted into a common header. Maybe __functional_base.
>> 
>> I think <__functional_base> is a good suggestion.  However I started investigating this, and found it unnecessary.  <dynarray> already must include <algorithm> which includes <memory>, which includes <tuple>.  I think it is good that <dynarray> includes <tuple> to make it explicit.  But we aren't going to save anything by migrating __uses_alloc_ctor.  
>> 
>> That being said, see my comments below about Marshall's new __user_alloc_construct facility, which ought to be moved out of <dynarray> and packaged with __uses_alloc_ctor.  I contradict what I'm saying here, there. :-)
>> 
>>> 2) This includes a general implementation of user-allocator construction (named __user_alloc_construct). This should probably be hoisted as well; and possibly the code in scoped_allocator can be reworked to use it (if it simplifies things).
>> 
>> I like this.  Thanks for factoring it out.  Add "::" to the use of new:
>> 
>>   ::new (__storage) _Tp (_VSTD::forward<_Args>(__args)...);
>> 
>> Add #include <new>, though it is already included implicitly anyway.
>> 
>> Hmm... logically this ought to go with __uses_alloc_ctor.  But it requires <new> and <__functional_base> does not have it.  I would rather not add <new> to <__functional_base>.  I would rather migrate __user_alloc_construct/__uses_alloc_ctor down the header dependency until <new> is available.
>> 
>> However when I just now tried to do so, I created circular header dependencies.  After investigating further, adding <new> to <__functional_base> isn't so bad.  Everything <new> includes is already included by <__functional_base>.  So <__functional_base> only swells by the immediate contents of <new> which is pretty small.
>> 
>> In summary, my suggestion is:
>> 
>> *  Add #include <new> to <__functional_base>
>> 
>> *  Move allocator_arg_t, __uses_alloc_ctor, __user_alloc_construct to <__functional_base>.
> 
> Ok, this is all done in a separate patch  (uses-alloc.patch; today at 8:19 AM)
> 
> 
>>> 4) There's an awful lot of duplication in the (many, many) constructors.
>> 
>> Why not make base_ a _Tp* instead of a void*?  I think this will safely avoid some casting around.
> 
> Done.
> 
>> size_ and base_ need macro protection (leading "__").
> 
> Done.
> 
>> Need to add "inline" to each external definition marked with _LIBCPP_INLINE_VISIBILITY.  The client is allowed to #define _LIBCPP_INLINE_VISIBILITY, and we should go ahead and mark those functions with inline for those cases we think they are small enough to be inlined.  For those cases (and there may be some) where they are too big to be inlined, mark with neither inline nor _LIBCPP_INLINE_VISIBILITY.
> 
> Done.
> 
>> 
>> The use of uninitialized_fill_n and uninitialized_copy_n are too heavy-weight here.  We don't need or want to introduce the try/catch clauses associated with these functions.  Just copy the ::new-loops out of these functions and be sure to increment size_ on each iteration (as you've already correctly done elsewhere).
> 
> Done.
> 
>> Need a :: on the new in dynarray(size_type __c).
> 
> Done - there and in other places, too.
> 
>> __user_alloc_construct should use template argument deduction instead of explicit template arguments.
>> 
>> __user_alloc_construct and __user_alloc_construct_impl needs decoration with inline _LIBCPP_INLINE_VISIBILITY.
>> 
>> Case 0 of __user_alloc_construct_impl, remove unused argument name "__a".
> 
> Done in uses-alloc.patch
> 
>> Need _LIBCPP_INLINE_VISIBILITY on the private default ctor, and on the private __allocate function.  Use of inline on the private __allocate is superfluous.
> 
> Done.
> 
> 
>> I /think/ _LIBCPP_INLINE_VISIBILITY is unnecessary on the deleted copy assignment.
> 
> Removed.
> 
>> The lack of a deallocation in ~dynarray() is giving me the heebie jeebies. ;-)  Hopefully this gets resolved when we move to stack allocation.
> 
> That's a bug. ;-)
> Fixed.
> 
>> You can use noexcept instead of _NOEXCEPT here as long as it stays under #if _LIBCPP_STD_VER > 11.  But I think this is nothing but an aesthetic comment.
> 
> Done.
> 
>> Also aesthetic:  Prefer use of value_type over T or _Tp when possible.
> 
> Done.
> 
> Richard wrote:
>> To my reading, __allocate is required to throw std::bad_array_length if the multiplication overflows.
> 
> Done.
> 
>> Your reinterpret_casts from void* to (const) _Tp* could be static_casts.
> 
> Done.
> 
>> The constructor overloads look... horribly broken. This won't work:
> 
> Per the discussion on the -lib reflector, I have commented out the Allocator-based constructors, and disabled the tests for them.
> They are marked with a comment "pending resolution of LWG isse #2235.
> 
> Note: This patch depends on other outstanding patches; specifically libcxx_bad_array_length.patch and uses-alloc.patch.

This looks pretty good to me.

Do we want to commit this without any stack allocation support though?  That still makes me nervous.  Without it, this is just another vector/valarray/unique_ptr<T[]>.  If we can't whip out some stack support for this in short order, and if no one else can, I think it should be pulled from C++1y.

On the other hand, we've got it under #if _LIBCPP_STD_VER > 11 and we ought to be able to pull it out prior to C++14 if things don't work out.  I guess this means it goes in for now.  But this really looks like an immature library to me.  Great implementation by Marshall, just immature in the spec and the field experience.

max_size():  I almost wrote up an issue on this.  But on reflection I decided not to.  I think the issue would resolve to:

    return numeric_limits<size_t>::max() / sizeof (value_type);

Let's just do that.  If anyone disagrees with that resolution, write up an LWG issue suggestion with what you think we should do, and then lets discuss that.

Outline (removing inline and _LIBCPP_INLINE_VISIBILITY) everything that throws, including __allocate() and both flavors of at().  Throwing an exception is a large source code cost.  For at() this is trivial, just remove the "inline _LIBCPP_INLINE_VISIBILITY" for what you have, both in the declaration and definition.  For __allocate(), move it outside the class declaration like at().

On ~dynarray():  Nice touch with the reverse order destruction.  I don't recall if you had this before or not, but I do remember thinking about suggesting it.  I like it.  We're doing this in vector today, and some people have actually favorably noticed.  One nit:

template <class _Tp>
inline _LIBCPP_INLINE_VISIBILITY
dynarray<_Tp>::~dynarray()
{ 
    value_type *__data = data () + __size_;
    for ( size_t i = 0; i < __size_; ++i, --__data )
        __data->value_type::~value_type();
    __deallocate ( __base_ );
}

Need to decrement __data prior to the destruction instead of after:

template <class _Tp>
inline _LIBCPP_INLINE_VISIBILITY
dynarray<_Tp>::~dynarray()
{ 
    value_type *__data = data () + __size_;
    for ( size_t i = 0; i < __size_; ++i )
    {
        --__data;
        __data->value_type::~value_type();
    }
    __deallocate ( __base_ );
}

And actually I might simply this down to:

template <class _Tp>
inline _LIBCPP_INLINE_VISIBILITY
dynarray<_Tp>::~dynarray()
{ 
    for (value_type* __data = __base_ + __size_; __data > __base_;)
    {
        --__data;
        __data->~value_type();
    }
    __deallocate ( __base_ );
}

But that much is just stylistic.  I leave that part up to you.

Please commit with these changes, and thanks much.  Nice job!

Clang team:  If we don't have at least some stack support by Chicago, I may recommend removing dynarray for lack of implementation experience.  I'm seeking feedback from the clang team on that course of action.  If I hear from you that such support is no problem and I'm just being a nervous nanny, I'll back down.  But if we're still figuring out how to do this, and no one else has either, then color me nervous nanny.  dynarray is not worthy of standardization without stack support.

Howard