libc++: First cut at <dynarray>

[Richard Smith]

On Mon, Sep 9, 2013 at 5:26 PM, Richard Smith <richard at metafoo.co.uk> wrote:

>
> On Mon, Sep 9, 2013 at 4:48 PM, Howard Hinnant <howard.hinnant at gmail.com>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 <mclow.lists at gmail.com> 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
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130909/aad7d1f1/attachment.html>