<memory>(mostly): Uses-Allocator and guaranteed copy elision For piecewise construction #421

ArtemSarmini · 2020-01-15T12:20:54Z

Description

Resolves #19 and resolves #20. I mostly copied standard quotes, unrolled some function calls and _Uglified. Now I wonder if it is possible to merge some code between uses_allocator_construction_args and _Uses_allocator_construct (from xpolymorphic_allocator.h), their purpose is same it seems.

Checklist

Be sure you've read README.md and understand the scope of this repo.

If you're unsure about a box, leave it unchecked. A maintainer will help you.

Identifiers in product code changes are properly _Ugly as per
https://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
The STL builds successfully and all tests have passed (must be manually
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
These changes introduce no known ABI breaks (adding members, renaming
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
These changes were written from scratch using only this repository,
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.

miscco

Some minor comments

miscco · 2020-01-15T20:34:54Z

stl/inc/scoped_allocator

+#if _HAS_CXX20
        _Uses_allocator_construct(_Ptr, _Scoped_outermost(*this), inner_allocator(), _STD forward<_Types>(_Args)...);
+#else // ^^^ _HAS_CXX20 / !_HAS_CXX20 vvv
+        _Uses_allocator_construct(_Ptr, _Scoped_outermost(*this), inner_allocator(), _STD forward<_Types>(_Args)...);


Maybe I am blind but arent thesetwo lines identical`?

#if part changed, as in standard, in terms of uses_allocator_construction_args

miscco · 2020-01-15T20:40:26Z

stl/inc/xmemory

+template <class>
+struct _Is_pair : false_type {};
+
+template <class _Ty1, class _Ty2>
+struct _Is_pair<pair<_Ty1, _Ty2>> : true_type {};


You are only ever using the value of _Is_pair so a variable template be better as that does not require a class instantiation

Suggested change

template <class>

struct _Is_pair : false_type {};

template <class _Ty1, class _Ty2>

struct _Is_pair<pair<_Ty1, _Ty2>> : true_type {};

// VARIABLE TEMPLATE _Is_pair_v

template <class>

inline constexpr bool _Is_pair_v = false;

template <class _Ty1, class _Ty2>

inline constexpr bool _Is_pair_v<pair<_Ty1, _Ty2>> = false;

miscco · 2020-01-15T20:54:39Z

stl/inc/xmemory

+auto uses_allocator_construction_args(const _Alloc& _Al, _Types&&... _Args) {
+    if constexpr (!uses_allocator_v<_Ty, _Alloc>) {
+        static_assert(is_constructible_v<_Ty, _Types...>,
+            "uses_allocator_v<_Ty, _Alloc> is false but _Ty is not constructible from _Types...");


Generally the error message uses the non-ugly names from the standard, as the user should not have to read the implementation to understand the message. Also I would suggest to formulate it differently along the lines of

T must be constructible from Types... if uses_allocator_v<T, Alloc> is false

Hmmm I think it depends on whether we expect a template instantiation stack to be shown to the user. If the compiler is going to add:

with [ _Ugly = sometype ]

to the output IMO it's more confusing to use the pretty name.

miscco · 2020-01-15T20:55:16Z

stl/inc/xmemory

+        return _STD forward_as_tuple(_STD forward<_Types>(_Args)...);
+    } else {
+        if constexpr (is_constructible_v<_Ty, allocator_arg_t, _Types...>) {
+            using _Return_type = _STD tuple<allocator_arg_t, const _Alloc&, _Types&&...>;


The _STD qualifier is unecessary as tuple is a type and not a function

miscco · 2020-01-15T20:56:42Z

stl/inc/xmemory

+            return _STD forward_as_tuple(_STD forward<_Types>(_Args)..., _Al);
+        } else {
+            static_assert(
+                _Always_false<_Ty>, "uses_allocator_v<_Ty, _Alloc> is true but _Ty is not constructible with _Alloc");


Similar to above, the names should be non ugly.

miscco · 2020-01-15T20:59:35Z

stl/inc/xmemory

+            _STD forward<_Tuple2>(_Tup2)));
+}
+
+template <class _Ty, class _Alloc, enable_if_t<_Is_pair<_Ty>::value, int> = 0>


Suggested change

template <class _Ty, class _Alloc, enable_if_t<_Is_pair<_Ty>::value, int> = 0>

template <class _Ty, class _Alloc, enable_if_t<_Is_pair_v<_Ty>, int> = 0>

miscco · 2020-01-15T21:00:32Z

stl/inc/xmemory

+        uses_allocator_construction_args<typename _Ty::second_type>(_Al));
+}
+
+template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>


Suggested change

template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>

template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair_v<_Ty>, int> = 0>

miscco · 2020-01-15T21:02:35Z

stl/inc/xmemory

+        uses_allocator_construction_args<typename _Ty::second_type>(_Al, _STD forward<_Uty2>(_Val2)));
+}
+
+template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>


Suggested change

template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>

template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair_v<_Ty>, int> = 0>

miscco · 2020-01-15T21:02:48Z

stl/inc/xmemory

+        uses_allocator_construction_args<typename _Ty::second_type>(_Al, _Pair.second));
+}
+
+template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>


Suggested change

template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>

template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair_v<_Ty>, int> = 0>

miscco · 2020-01-15T21:06:35Z

stl/inc/xmemory

+
+template <class _Ty, class _Alloc, enable_if_t<_Is_pair<_Ty>::value, int> = 0>
+auto uses_allocator_construction_args(const _Alloc& _Al) {
+    // equivalent to


Why are you not returning whats the equivalent call? This applies to all functions. As I can see it it involves more function calls.

Isn't it good to have less function template instantiations? And no, it involves one less uses_allocator_construction_args and 2 apply calls in all cases

AdamBucior · 2020-01-22T16:17:42Z

This also requires changes to <yvals_core.h>.

AdamBucior · 2020-01-22T16:34:58Z

stl/inc/xmemory

+template <class _Ty1, class _Ty2>
+inline constexpr bool _Is_pair_v<pair<_Ty1, _Ty2>> = true;
+
+template <class _Ty, class _Alloc, class... _Types, enable_if_t<!_Is_pair_v<_Ty>, int> = 0>


It's a c++20 feature. Can't we use concepts there instead of SFINAE?

Not sure if it is allowed, in the standard template parameters are plain classes. Plus we'd need 2 concepts, Pair and NotPair. Anyway, I think this sfinae is rather simple, but I'd rewrite it if concepts are faster in terms of compilation speed

We don't need to make any concepts we can just use requires keyword.

Moreover the standard uses the word Constraints

Constraints: T is not a specialization of pair.

which evidently suggests the use of concepts.

I think concepts are generally preferable in newer code as they are simpler and provide better error messages.
Nevertheless whether you use a concept or SFINAE is your choice.

SFINAE is horrible. Working with it is one of my biggest nightmares.

It's a c++20 feature. Can't we use concepts there instead of SFINAE?

Sadly not all the frontends we support have concepts yet.

SFINAE is horrible. Working with it is one of my biggest nightmares.

concepts are mostly SFINAE :)

Fair. I'll keep _Is_pair and replace enable_ifs with requires

Sadly not all the frontends we support have concepts yet.

Yet. Someday they will.

concepts are mostly SFINAE :)

But 100x simpler and easier to debug.

Sadly not all the frontends we support have concepts yet.

So enable_if remains for now?

ArtemSarmini · 2020-01-22T16:37:50Z

Sorry, wrong branch

BillyONeal · 2020-01-15T22:32:51Z

stl/inc/xmemory

+#if _HAS_CXX20
+
+template <class>
+struct _Is_pair : false_type {};


I think this is _Is_specialization<T, pair>.

Didn't knew this existed, will try

BillyONeal · 2020-01-15T22:34:22Z

stl/inc/xmemory

+auto uses_allocator_construction_args(const _Alloc& _Al, _Types&&... _Args) {
+    if constexpr (!uses_allocator_v<_Ty, _Alloc>) {
+        static_assert(is_constructible_v<_Ty, _Types...>,
+            "uses_allocator_v<_Ty, _Alloc> is false but _Ty is not constructible from _Types...");


Hmmm I think it depends on whether we expect a template instantiation stack to be shown to the user. If the compiler is going to add:

with [ _Ugly = sometype ]

to the output IMO it's more confusing to use the pretty name.

BillyONeal · 2020-01-22T19:38:54Z

stl/inc/xmemory

+template <class _Ty1, class _Ty2>
+inline constexpr bool _Is_pair_v<pair<_Ty1, _Ty2>> = true;
+
+template <class _Ty, class _Alloc, class... _Types, enable_if_t<!_Is_pair_v<_Ty>, int> = 0>


It's a c++20 feature. Can't we use concepts there instead of SFINAE?

Sadly not all the frontends we support have concepts yet.

SFINAE is horrible. Working with it is one of my biggest nightmares.

concepts are mostly SFINAE :)

StephanTLavavej · 2020-01-23T02:51:38Z

/azp run

azure-pipelines · 2020-01-23T02:51:47Z

Azure Pipelines successfully started running 1 pipeline(s).

CaseyCarter · 2020-04-15T20:00:59Z

Looks like there are some merge conflicts to resolve, and the new functionality needs some test coverage.

StephanTLavavej · 2020-05-27T20:20:05Z

Ping - would you be able to add test coverage here? If not, we can take care of getting this PR ready to merge.

StephanTLavavej · 2021-02-22T04:38:13Z

Thanks for implementing these features - and apologies for not reviewing this earlier. @AdamBucior has picked this up in #1668 with test coverage, so we'll continue with that PR.

'WIP'

bc81f4a

ArtemSarmini requested a review from a team as a code owner January 15, 2020 12:20

miscco reviewed Jan 15, 2020

View reviewed changes

ArtemSarmini added 2 commits January 16, 2020 03:49

'Fixes'

0d4acf5

Fixed format in scoped_allocator_adaptor::construct

ed041ad

AdamBucior reviewed Jan 22, 2020

View reviewed changes

yvals comment

e9fd2f2

yvals comment fixed

78b8110

BillyONeal reviewed Jan 22, 2020

View reviewed changes

_Is_pair replaced with existed _Is_specialization

312cf73

ArtemSarmini requested a review from BillyONeal January 27, 2020 11:33

StephanTLavavej added the cxx20 C++20 feature label Feb 5, 2020

Base automatically changed from master to main January 28, 2021 00:35

AdamBucior mentioned this pull request Feb 20, 2021

Uses-Allocator and guaranteed copy elision For piecewise construction #1668

Merged

StephanTLavavej closed this Feb 22, 2021

	template <class _Ty, class _Alloc, enable_if_t<_Is_pair<_Ty>::value, int> = 0>
	template <class _Ty, class _Alloc, enable_if_t<_Is_pair_v<_Ty>, int> = 0>

	template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair<_Ty>::value, int> = 0>
	template <class _Ty, class _Alloc, class _Uty1, class _Uty2, enable_if_t<_Is_pair_v<_Ty>, int> = 0>

<memory>(mostly): Uses-Allocator and guaranteed copy elision For piecewise construction #421

<memory>(mostly): Uses-Allocator and guaranteed copy elision For piecewise construction #421

Uh oh!

Conversation

ArtemSarmini commented Jan 15, 2020 • edited by StephanTLavavej Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

miscco 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!

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!

ArtemSarmini Jan 16, 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!

AdamBucior commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtemSarmini Jan 22, 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!

AdamBucior Jan 22, 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!

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!

ArtemSarmini commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

StephanTLavavej commented Jan 23, 2020

Uh oh!

azure-pipelines bot commented Jan 23, 2020

ArtemSarmini commented Jan 15, 2020 •

edited by StephanTLavavej

Loading

ArtemSarmini Jan 16, 2020 •

edited

Loading

ArtemSarmini Jan 22, 2020 •

edited

Loading

AdamBucior Jan 22, 2020 •

edited

Loading

ArtemSarmini commented Jan 22, 2020 •

edited

Loading