On Mon, Sep 9, 2013 at 5:26 PM, Richard Smith <[email protected]> wrote:
> > On Mon, 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. > > > I don't think that's possible (and Nick has argued that it's not > desirable, because it will probably be a pessimization; I don't have data > there). > > Consider: > > void g(std::dynarray<int> &); > > void f() { > Umm... "int f() {" =) > std::dynarray<int> arr(10); > g(arr); > return arr[3]; > } > > // Some other TU: > void g(std::dynarray<int> &arr) { > arr.~std::dynarray<int>(); > new (&arr) std::dynarray<int>(30); > } > > I believe this code is valid, and we can't promote the allocation in 'f' > to the stack, nor the allocation in 'g'. > > Also, if the dynarray object itself is on the heap, or in a global or > thread-local variable, we can't promote its allocation to the stack. And if > it's in a class type that's on the stack, we can only promote that if we > manage to inline the constructor and destructor (and we see that the > allocation doesn't escape). > > You should think of putting the allocation on the stack as, at best, an > optimization, and should expect the allocation to go on the heap if we > cannot perform the optimization. FWIW, I'm not really sure why we have > std::dynarray, rather than an allocator for std::vector that encourages a > heap -> stack optimization. > > >> 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
