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. 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>. > > 3) There's no support for stack allocations at present; that requires > compiler support. I'm working with some people on getting that into clang. I'm guessing those people are cc'd here. :-) > > 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. size_ and base_ need macro protection (leading "__"). 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. 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). Need a :: on the new in dynarray(size_type __c). __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". Need _LIBCPP_INLINE_VISIBILITY on the private default ctor, and on the private __allocate function. Use of inline on the private __allocate is superfluous. I /think/ _LIBCPP_INLINE_VISIBILITY is unnecessary on the deleted copy assignment. The lack of a deallocation in ~dynarray() is giving me the heebie jeebies. ;-) Hopefully this gets resolved when we move to stack allocation. 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. Also aesthetic: Prefer use of value_type over T or _Tp when possible. Looking really good! Looking forward to seeing the stack allocator. Thanks, Howard _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
