<memory> construct_at() #501

AdamBucior · 2020-02-13T20:05:30Z

Description

A small and quite useful part of #37.

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.

Update

stl/inc/memory

stl/inc/yvals_core.h

BillyONeal · 2020-02-20T00:50:12Z

I don't think there should be any conditional compilation for the constexpr new thing, since in that case the construct_at call itself is likely to be needed to be treated as special by compilers.

AdamBucior · 2020-02-20T16:41:06Z

I don't think there should be any conditional compilation for the constexpr new thing, since in that case the construct_at call itself is likely to be needed to be treated as special by compilers.

So should I just make it constexpr unconditionally even though none of the compilers supports constexpr new yet? Or should I place there /* constexpr */ comment? I think I don't understand.

BillyONeal · 2020-02-20T23:41:15Z

I would not declare constexpr anything here.

stl/inc/memory

… and destroy_at, and add test.

BillyONeal · 2020-02-27T20:00:07Z

@AdamBucior Are you happy with my changes?

AdamBucior · 2020-02-27T20:03:32Z

Yes, thanks!

stl/inc/memory

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

BillyONeal · 2020-02-28T06:08:20Z

I might be pedantic here but it greatly improves readability if there is a space before the noexcepts so that they align with the false tests
static_assert( noexcept(std::construct_at(std::declval<int*>(), 42)));
static_assert(!noexcept(std::construct_at(std::declval<std::string*>(), "hello")));

clang-format hates this.

CaseyCarter · 2020-02-28T15:53:37Z

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

+
+struct X {};
+
+template <class _Void, class Ty, class... Types>


_Ugly.

Suggested change

template <class _Void, class Ty, class... Types>

template <class Void, class Ty, class... Types>

Since I'm here typing, do you think it would help readability to add this void in an intermediate layer and unclutter the call sites?

CaseyCarter · 2020-02-28T15:59:57Z

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

+static_assert(!can_construct_at<void, int, X>);
+static_assert(!can_construct_at<void, X, int>);
+
+static_assert(!can_construct_at<void, int&>); // note that references can be constructed by not new'd


"by" should be "but". I also find "new'd" an odd contraction. Maybe "note that new only works with object types"?

Actually, it seems a bit silly to test reference types here given that the function deduces the type from a pointer argument and there are no pointers to references. Function types would be a bit more reasonable - I can at least form a pointer to a function to pass to construct_at.

Hmmm I was trying to put something in here that would indicate use of the new SFINAE rather than is_constructible... but maybe calling is_constructible would be correct?

I think can_construct_at is very clearly not is_constructible_v even without the void.

maybe calling is_constructible would be correct?

Nope: it's very intentionally "if this new expression is well-formed" instead of is_constructible because the latter requires destructibility which the former does not. (I'm not a fan of this level of constraint minimality, but that's what we standardized.)

Replaced these reference tests with struct indestructible.

stl/inc/memory

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

StephanTLavavej · 2020-02-29T00:30:07Z

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

+inline constexpr bool
+    can_construct_at_impl<void_t<decltype(construct_at(declval<Ty*>(), declval<Types>()...))>, Ty, Types...> = true;
+
+template<class Ty, class... Types>


This is failing clang-format. I recommend configuring your editor to format on save.

Unfortunately I can't use format-on-save because I spend too much time editing things that aren't the STL that are damaged when I do that :(

(I'm quite happy to have our CI yell at me :) )

Please consider using code . as

STL/.vscode/settings.json

Line 4 in d4ee5c3

"editor.formatOnSave": true,

will set format-on-save for just this workspace.

stl/inc/memory

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

BillyONeal · 2020-02-29T03:19:50Z

Thanks again for your contribution!

<memory> construct_at() (microsoft#501)

AdamBucior · 2020-02-29T10:03:30Z

Thanks again for your contribution!

Thanks again for your changes and review!

AdamBucior and others added 3 commits December 5, 2019 17:49

Merge pull request #2 from microsoft/master

e3c45e5

Update

Merge remote-tracking branch 'upstream/master'

a494d24

Implement construct_at()

2f1b855

AdamBucior requested a review from a team as a code owner February 13, 2020 20:05

miscco reviewed Feb 14, 2020

View reviewed changes

stl/inc/memory Outdated Show resolved Hide resolved

miscco reviewed Feb 14, 2020

View reviewed changes

stl/inc/memory Outdated Show resolved Hide resolved

Fix pointer conversion

aa7a66a

miscco reviewed Feb 15, 2020

View reviewed changes

stl/inc/memory Outdated Show resolved Hide resolved

_CONSTEXPR_NEW

2ccb7ca

viccpp reviewed Feb 19, 2020

View reviewed changes

stl/inc/yvals_core.h Outdated Show resolved Hide resolved

Improve debug performance of ranges version

59ebd80

Remove _CONSTEXPR_NEW

09cfb86

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

StephanTLavavej assigned BillyONeal Feb 26, 2020

BillyONeal reviewed Feb 27, 2020

View reviewed changes

stl/inc/memory Outdated Show resolved Hide resolved

BillyONeal added 3 commits February 27, 2020 11:39

Merge remote-tracking branch 'origin/master' into construct_at

9e1a24e

Deduplicate the "ranges" version, strengthen noexcept on construct_at…

5eb0ae6

… and destroy_at, and add test.

Fix test syntax errors.

83007e8

BillyONeal approved these changes Feb 27, 2020

View reviewed changes

StephanTLavavej requested changes Feb 27, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into construct_at

d78a788

Add constraints and other CR feedback.

8877a1a

BillyONeal requested a review from StephanTLavavej February 28, 2020 07:10

CaseyCarter suggested changes Feb 28, 2020

View reviewed changes

StephanTLavavej requested changes Feb 28, 2020

View reviewed changes

More CR feedback, test indestructible.

e49387b

StephanTLavavej approved these changes Feb 29, 2020

View reviewed changes

CaseyCarter approved these changes Feb 29, 2020

View reviewed changes

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp Show resolved Hide resolved

clang-format

238205f

StephanTLavavej approved these changes Feb 29, 2020

View reviewed changes

BillyONeal merged commit a092e67 into microsoft:master Feb 29, 2020

fengjixuchui added a commit to fengjixuchui/STL that referenced this pull request Feb 29, 2020

Merge pull request #42 from microsoft/master

877bb8f

<memory> construct_at() (microsoft#501)

AdamBucior deleted the construct_at branch February 29, 2020 10:04

MichaelRizkalla mentioned this pull request Oct 14, 2020

<xmemory> Add constexpr to allocator traits #1369

Merged

3 tasks


		struct X {};

		template <class _Void, class Ty, class... Types>

	template <class _Void, class Ty, class... Types>
	template <class Void, class Ty, class... Types>

<memory> construct_at() #501

<memory> construct_at() #501

Uh oh!

Conversation

AdamBucior commented Feb 13, 2020 • edited by BillyONeal Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BillyONeal commented Feb 20, 2020

Uh oh!

AdamBucior commented Feb 20, 2020

Uh oh!

BillyONeal commented Feb 20, 2020

Uh oh!

Uh oh!

BillyONeal commented Feb 27, 2020

Uh oh!

AdamBucior commented Feb 27, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BillyONeal commented Feb 28, 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!

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

BillyONeal commented Feb 29, 2020

Uh oh!

AdamBucior commented Feb 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

AdamBucior commented Feb 13, 2020 •

edited by BillyONeal

Loading