libc++: First cut at <dynarray>

[Richard Smith]

On Thu, Sep 12, 2013 at 6:23 PM, Howard Hinnant <howard.hinnant at gmail.com>wrote:

> 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();
>

Hmm, what should size() return if it's called during the destruction of the
dynarray? During construction, it returns the number of fully-constructed
elements. Maybe we should do the same here:

dynarray<_Tp>::~dynarray()
{
  value_type* __data = data();
  size_type __size = size();
  while ( __size )
  {
    __size_ = --__size;
    __data[__size].~value_type();
  }
  __deallocate( __base_ );
}

    }
>     __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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130912/31923135/attachment.html>