----- Original Message ----- > 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(); > } > __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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > -- Hal Finkel Assistant Computational Scientist Leadership Computing Facility Argonne National Laboratory _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
