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. -- Marshall Marshall Clow Idio Software <mailto:[email protected]> A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait). -- Yu Suzuki
dynarray-2.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
