On Thu, Sep 12, 2013 at 6:23 PM, Howard Hinnant <[email protected]>wrote:
> On Sep 10, 2013, at 12:38 PM, Marshall Clow <[email protected]> wrote: > > > On Sep 9, 2013, at 4:48 PM, Howard Hinnant <[email protected]> > 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 <[email protected]> > 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 > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
