Smart Pointer Creation With Default Initialization #778

ggweinand · 2020-05-01T23:10:00Z

Resolves #48. There are some comments throughout about minor wording/formatting doubts I have, otherwise I believe it should be complete.

stl/inc/memory: The reference count blocks got an additional non type template parameter of the new enum class _Init_form, which specifies the initialization form. make_shared_for_overwrite and allocate_shared_for_overwrite, as well as helper functions for default-initing were implemented.
stl/inc/xmemory: _Construct_in_place_for_overwrite, a helper function, was based on _Construct_in_place. The new one does default-init.
tests/std/tests/P1020R1_smart_pointer_for_overwrite/test.cpp : Added test coverage for make_shared_for_overwrite and allocate_shared_for_overwrite.
stl/inc/yvals_core.h: Added the feature test macro __cpp_lib_smart_ptr_for_overwrite for cxx20 mode.
tests/std/tests/VSO_0157762_feature_test_macros/test.cpp: Added test coverage for __cpp_lib_smart_ptr_for_overwrite in cxx20 mode.

AdamBucior

Some comments about the product code changes.

AdamBucior · 2020-05-02T08:55:54Z

stl/inc/memory

+// FUNCTION TEMPLATE make_unique_for_overwrite
+template <class _Ty, enable_if_t<!is_array_v<_Ty>, int> = 0>
+_NODISCARD unique_ptr<_Ty> make_unique_for_overwrite() { // make a unique_ptr with default initialization
+    return unique_ptr<_Ty>(new _Ty);
+}
+
+template <class _Ty, enable_if_t<is_array_v<_Ty> && extent_v<_Ty> == 0, int> = 0>
+_NODISCARD unique_ptr<_Ty> make_unique_for_overwrite(size_t _Size) { // make a unique_ptr with default initialization
+    using _Elem = remove_extent_t<_Ty>;
+    return unique_ptr<_Ty>(new _Elem[_Size]);
+}
+
+template <class _Ty, class... _Types, enable_if_t<extent_v<_Ty> != 0, int> = 0>
+void make_unique_for_overwrite(_Types&&...) = delete;
+


This should be guarded with _HAS_CXX20.

AdamBucior · 2020-05-02T08:57:29Z

stl/inc/memory

+// Should this be in a separate ifdef _HAS_CXX20 control block since it is a
+// different feature?


Pretty sure it shouldn't.

AdamBucior · 2020-05-02T08:58:14Z

stl/inc/memory

 }
 #endif // _HAS_CXX20

+// Once more, should this be in a separate #ifdef _HAS_CXX20 block?


AdamBucior · 2020-05-02T09:12:25Z

stl/inc/memory

+
 // CLASS TEMPLATE _Ref_count_obj2
-template <class _Ty>
+template <class _Ty, _Init_form _Init>


Wouldn't it be better to just introduce a new constructor taking some tag type? It's a polymorphic class so each instantiation needs to have it's own vtable. Ditto with all _Ref_count_*s.

I agree it would be better. Let me see what I can come up with.

AdamBucior · 2020-05-02T09:14:51Z

stl/inc/memory

 }

-template <class _NoThrowIt>
+template <class _NoThrowIt, _Init_form _Init>


Similarly, maybe just introduce a new member function. Something like _Push_back_for_overwrite.

miscco

Looks good from a short skimm, I would second @AdamBucior wrt the additional template arg

miscco · 2020-05-02T11:39:16Z

stl/inc/memory

+    template <class _Ty0>
+    friend enable_if_t<!is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> make_shared_for_overwrite();
+
+    template <class _Ty0, class _Alloc>
+    friend enable_if_t<!is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> allocate_shared_for_overwrite(
+        const _Alloc& _Al_arg);
+
+    template <class _Ty0>
+    friend enable_if_t<is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> make_shared_for_overwrite(size_t _Count);
+
+    template <class _Ty0, class _Alloc>
+    friend enable_if_t<is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> allocate_shared_for_overwrite(
+        const _Alloc& _Al_arg, size_t _Count);


This is contrary to the "usual" way enable_if is used in the code. I would expect something along those lines:

Suggested change

template <class _Ty0>

friend enable_if_t<!is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> make_shared_for_overwrite();

template <class _Ty0, class _Alloc>

friend enable_if_t<!is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> allocate_shared_for_overwrite(

const _Alloc& _Al_arg);

template <class _Ty0>

friend enable_if_t<is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> make_shared_for_overwrite(size_t _Count);

template <class _Ty0, class _Alloc>

friend enable_if_t<is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> allocate_shared_for_overwrite(

const _Alloc& _Al_arg, size_t _Count);

template <class _Ty0, enable_if_t<!is_unbounded_array_v<_Ty0>, int> = 0>

friend shared_ptr<_Ty0> make_shared_for_overwrite();

template <class _Ty0, class _Alloc, enable_if_t<!is_unbounded_array_v<_Ty0>, int> = 0>

friend shared_ptr<_Ty0> allocate_shared_for_overwrite(

const _Alloc& _Al_arg);

template <class _Ty0, enable_if_t<is_unbounded_array_v<_Ty0>, int> = 0>

friend shared_ptr<_Ty0> make_shared_for_overwrite(size_t _Count);

template <class _Ty0, class _Alloc, enable_if_t<is_unbounded_array_v<_Ty0>, int> = 0>

friend shared_ptr<_Ty0> allocate_shared_for_overwrite(

const _Alloc& _Al_arg, size_t _Count);

It seemed a bit weird to me too, but I tried to copy the style used in make_shared.

EDIT: Here is an example.

template <class _Ty0, class... _Types> friend enable_if_t<!is_array_v<_Ty0>, shared_ptr<_Ty0>> make_shared(_Types&&... _Args);

This was my doing in #309, where I took @AdamBucior's elegant concepts-based code and reimplemented it with ~~neolithic stone tools~~ SFINAE, because not all of our supported compilers have implemented concepts yet (e.g. IntelliSense). While we ordinarily use SFINAE in a highly consistent manner as @miscco noted, it doesn't work here 😿 due to the friendship (plus interaction with compiler bugs, IIRC). I think I was a bad kitty - instead of reducing and reporting the compiler bug, I perma-worked-around it (since default arg SFINAE would have needed verbose forward declarations anyways) by using ~~paleolithic stone tools~~ return type SFINAE.

(The issue with template argument SFINAE was whether friends can introduce default template arguments.)

I doubled down on my bad kitty behavior 😼 by ~~jumping up on the table where I'm not allowed~~ not marking this workaround as TRANSITION, because the SFINAE is ultimately doomed. In the future, when our C++20 compilers all support concepts, we can eradicate this and go back to Adam's original, elegant code.

MikeGitb · 2020-05-02T08:15:18Z

stl/inc/memory

+// different feature?
+#if _HAS_CXX20
+    template <class _Ty0>
+    friend enable_if_t<!is_unbounded_array_v<_Ty0>, shared_ptr<_Ty0>> make_shared_for_overwrite();


Could this use concepts?

Not all supported frontends support concepts. There is an issue #602 to change it later.

ggweinand · 2020-05-07T00:39:06Z

Maybe I'm missing something. If I change change the constructor to take an extra argument of type _Init_form then I lose the possibility of using if constexpr. Is that what you want me to do?

AdamBucior · 2020-05-07T07:42:57Z

Maybe I'm missing something. If I change change the constructor to take an extra argument of type _Init_form then I lose the possibility of using if constexpr. Is that what you want me to do?

The best way would be to make one constructor take a tag type i.e. create a type like class _For_overwrite_tag {};, make a constructor that takes this type and in make_shared_for_overwrite construct ref counts using an instance of this type.

StephanTLavavej · 2020-05-08T23:51:31Z

tests/std/test.lst

 tests\P0919R3_heterogeneous_unordered_lookup
 tests\P0966R1_string_reserve_should_not_shrink
 tests\P1023R0_constexpr_for_array_comparisons
+tests\P1020R1_smart_pointer_for_overwrite


This should appear in sorted order.

StephanTLavavej · 2020-05-08T23:52:56Z

stl/inc/yvals_core.h

 #define __cpp_lib_math_constants          201907L
 #define __cpp_lib_remove_cvref            201711L
 #define __cpp_lib_shift                   201806L
+#define __cpp_lib_smart_ptr_for_overwrite 201811L


This feature should also be mentioned in the list of _HAS_CXX20 features at the top of yvals_core.h.

StephanTLavavej · 2020-05-08T23:58:05Z

stl/inc/memory

-template <class _Ty, class _Alloc>
-class _Ref_count_obj_alloc3 : public _Ebco_base<_Rebind_alloc_t<_Alloc, _Ty>>, public _Ref_count_base {
+template <class _Ty, class _Alloc, _Init_form _Init>
+class __declspec(empty_bases) _Ref_count_obj_alloc3 : public _Ebco_base<_Rebind_alloc_t<_Alloc, _Ty>>,


We recently removed __declspec(empty_bases) from all control blocks, after noticing that it didn't actually have any effect (only _Ebco_base might be empty; _Ref_count_base never is), and it was causing compiler errors with CUDA 9.2. The 3 occurrences of __declspec(empty_bases) being added here should be removed again.

AdamBucior · 2020-09-23T19:11:15Z

Is it still worked on?

CaseyCarter · 2020-09-24T02:07:52Z

Is it still worked on?

I think Gianni got busy after his internship ended and he hasn't had time to work on this. Someone will need to pickup the branch and finish it off - it hasn't quite managed to reach critical mass yet for one of the maintainers to do so.

If someone else wants to copy Gianni's branch into their fork and finish this feature up, it would be amazing ;)

AdamBucior · 2020-09-24T06:45:11Z

If someone else wants to copy Gianni's branch into their fork and finish this feature up, it would be amazing ;)

I will work on this ;)

AdamBucior · 2020-09-24T14:36:36Z

Continuing this in #1315.

StephanTLavavej · 2020-09-24T22:12:06Z

Thanks again for starting this feature!

Ported my work from msvc.

f994009

ggweinand requested a review from StephanTLavavej May 1, 2020 23:10

ggweinand requested a review from a team as a code owner May 1, 2020 23:10

StephanTLavavej added the cxx20 C++20 feature label May 1, 2020

Used the wrong if constexpr macro.

39aa685

AdamBucior reviewed May 2, 2020

View reviewed changes

miscco reviewed May 2, 2020

View reviewed changes

MikeGitb reviewed May 2, 2020

View reviewed changes

StephanTLavavej requested changes May 8, 2020

View reviewed changes

mnatsuhara assigned CaseyCarter Jun 17, 2020

CaseyCarter removed their assignment Jul 29, 2020

AdamBucior mentioned this pull request Sep 24, 2020

Implement Smart Pointer Creation With Default Initialization #1315

Merged

StephanTLavavej pushed a commit to AdamBucior/STL that referenced this pull request Sep 24, 2020

microsoftGH-1315 is based on microsoftGH-778 by @Weheineman.

d57e8a6

StephanTLavavej closed this Sep 24, 2020

		// Should this be in a separate ifdef _HAS_CXX20 control block since it is a
		// different feature?

Smart Pointer Creation With Default Initialization #778

Smart Pointer Creation With Default Initialization #778

Uh oh!

Conversation

ggweinand commented May 1, 2020

Uh oh!

AdamBucior left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggweinand May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggweinand commented May 7, 2020

Uh oh!

AdamBucior commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdamBucior commented Sep 23, 2020

Uh oh!

CaseyCarter commented Sep 24, 2020

Uh oh!

AdamBucior commented Sep 24, 2020

Uh oh!

AdamBucior commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

StephanTLavavej commented Sep 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ggweinand May 6, 2020 •

edited

Loading

AdamBucior commented Sep 24, 2020 •

edited

Loading