libc++: First cut at <dynarray>

[Marshall Clow]

On 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.  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 <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>.

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:mclow.lists at gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly moderated down to (-1, Flamebait).
        -- Yu Suzuki

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dynarray-2.patch
Type: application/octet-stream
Size: 36463 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130910/e6dc0092/attachment.obj>