libc++: First cut at <dynarray>
Hal Finkel
hfinkel at anl.gov
Thu Sep 12 19:02:43 PDT 2013
----- Original Message -----
> 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.
Out of curiosity, what compiler support is needed? It seems to me that what you need is a variant of alloca that allocates memory in the caller's stack frame, correct? If we restrict this to functions marked always_inline, this certainly seems simple to implement and reasonable. If not, what did you have in mind?
-Hal
>
> Howard
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the cfe-commits
mailing list