<xmemory> Add constexpr to allocator traits #1369

MichaelRizkalla · 2020-10-14T00:13:57Z

Description

A small update that adds _CONSTEXPR20_DYNALLOC to allocator traits member functions to implement constexpr allocators when the compiler defines __cpp_constexpr_dynamic_alloc.

This pull request implements the default allocator changes in P0784R7 ~~except for [allocator.traits.members] construct and destruct Effects~~.

Useful for #37

Checklist

Before submitting a pull request, please ensure that:

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.
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.).
Your changes are 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 your changes are derived from a project that's already listed in NOTICE.txt, that's fine, but please mention it. If your changes are derived from any other project, you must mention it here, so we can determine whether the license is compatible and what else needs to be done.

ghost · 2020-10-14T00:14:16Z

All CLA requirements met.

stl/inc/memory

stl/inc/xmemory

stl/inc/yvals_core.h

stl/inc/xmemory

miscco · 2020-10-14T11:54:46Z

As @AdamBucior said, you need to add tests to tests\std\tests\P0784R7_library_support_for_more_constexpr_containers\test.cpp

Also as far as I know this would finish up P0784R7, so we should mark that in <yvals_core> as done

miscco · 2020-10-14T11:56:33Z

Not I have a wip branch lying around that essentially sprinkels constexpr all around vector.

The problem that we have is that it is not clear currently what to do about ::operator new and ::operator delete if they take the alignment parameter, because those are at least in clang not constexpr

MichaelRizkalla · 2020-10-14T12:44:25Z

As @AdamBucior said, you need to add tests to tests\std\tests\P0784R7_library_support_for_more_constexpr_containers\test.cpp

Also as far as I know this would finish up P0784R7, so we should mark that in <yvals_core> as done

I'll add test coverage asap.

There are two missing changes for P0784R7 to be marked it as finished.
In [allocator.traits.members] paragraph 5 and 6:
allocator_trait::construct should invoke construct_at(p, std::forward<Args>(args)...) if the call is not well-formed
allocator_trait::destroy should invoke destroy_at(p) if the call is not well-formed

construct_at and destroy_at are defined in memory header file in #501, so they would not be visible to allocator_traits.
It seemed reasonable to move construct_at and destroy_at to xmemory but since I am new here, I decided to avoid moving functions around until a complete dev guide comes out :)

miscco · 2020-10-14T13:49:21Z

I am moving construct_at in a3e3dc8

tests/std/tests/VSO_0157762_feature_test_macros/test.cpp

stl/inc/xmemory

mnatsuhara

Thanks for your work here! It looks like there are some merge conflicts right now which is preventing the test run from kicking off. I'll take another look once you have a chance to resolve the conflicts and address the review comments!

stl/inc/xmemory

stl/inc/yvals_core.h

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

miscco

There are some correctness concerns regarding the special alignment changes we do

This suggests that there are insufficient tests there.

Also I am surprised that this works for sized deallocation, as that did not compile in ä1407 Cna you double check that these code paths are tested

Finaly, I am quite surprised that non of the libcxx tests are green now. Did you forget to mark them as passing for clang?

stl/inc/xmemory

miscco

I am sorry for bombarding you with comments :(

stl/inc/xmemory

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

miscco

Last comment, but after resolving what the maintainers prefer for changing the allocators this looks great

stl/inc/xmemory

This is Following P0784R7.

Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>

…stexpr Changes as per the code review

Apply review suggestion Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>

Tests check if allocation tasks can be completed at compile time.

Again

This commit assumes a3e3dc8 is merged first

- Removing tests that are wrongly written

mnatsuhara

Just a small nitpick in test.cpp and a question of consistency given the recent change with feature-test macros 🐱

stl/inc/xmemory

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

mnatsuhara

Awesome, thanks for all of your work and follow up here! This is now ready for final review!

mnatsuhara · 2021-01-06T23:16:40Z

FYI @MichaelRizkalla, I merged your branch with master to resolve the merge conflicts after #1544 was checked in!

StephanTLavavej

Thanks, this looks good! I had a number of comments but they're all straightforward to resolve; in the interests of time I will prepare a commit to address them.

stl/inc/memory

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

StephanTLavavej · 2021-01-07T04:08:20Z

tests/std/tests/P0784R7_library_support_for_more_constexpr_containers/test.cpp

+        Alloc<A<int>> alloc{10};
+        assert(alloc.id == 10);
+
+        auto result = std::allocator_traits<Alloc<A<int>>>::allocate(alloc, 10);


Observation, no change requested: For clarity, I recommend using distinct values for distinct purposes in test code. Here, 10 is being used for the allocator's ID, and 10 is also the number of A<int> elements being allocated. This makes it more difficult to glance at the source code and immediately recognize the meaning of a 10.

StephanTLavavej · 2021-01-07T05:48:51Z

@MichaelRizkalla @mnatsuhara @miscco I believe this is ready to merge, but if you see any issues with the changes I just pushed (particularly the compiler guard changes), we can continue iterating.

stl/inc/xmemory

StephanTLavavej · 2021-01-08T03:03:46Z

It's merged! This will ship in VS 2019 16.10 Preview 1. Thanks for implementing this C++20 feature, and for enduring our exceptionally lengthy code review process, and congratulations on your first microsoft/STL commit! 🎉 🚀 😸

MichaelRizkalla requested a review from a team as a code owner October 14, 2020 00:13

StephanTLavavej added the cxx20 C++20 feature label Oct 14, 2020

AdamBucior reviewed Oct 14, 2020

View reviewed changes

CaseyCarter assigned CaseyCarter and mnatsuhara and unassigned CaseyCarter Oct 14, 2020

AdamBucior reviewed Oct 15, 2020

View reviewed changes

tests/std/tests/VSO_0157762_feature_test_macros/test.cpp Outdated Show resolved Hide resolved

miscco reviewed Nov 5, 2020

View reviewed changes

stl/inc/xmemory Outdated Show resolved Hide resolved

mnatsuhara suggested changes Nov 12, 2020

View reviewed changes

miscco reviewed Nov 13, 2020

View reviewed changes

stl/inc/xmemory Show resolved Hide resolved

stl/inc/xmemory Show resolved Hide resolved

stl/inc/xmemory Show resolved Hide resolved

stl/inc/xmemory Outdated Show resolved Hide resolved

miscco reviewed Nov 25, 2020

View reviewed changes

miscco approved these changes Nov 26, 2020

View reviewed changes

stl/inc/xmemory Show resolved Hide resolved

MichaelRizkalla requested a review from mnatsuhara November 27, 2020 09:59

MichaelRizkalla and others added 12 commits December 9, 2020 20:22

Add support for more constexpr containers

0b94bc2

This is Following P0784R7.

Conform to clang-format.

ac7ef50

Apply suggestions from code review

4255a9e

Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>

_Default_allocator_traits members, _Allocate and _Deallocate made con…

7276cec

…stexpr Changes as per the code review

Conform to clang-format

31425fd

correctly place __cpp_lib_constexpr_dynamic_alloc and add feature test

822be04

Enforce clang-format

45d1234

Update tests/std/tests/VSO_0157762_feature_test_macros/test.cpp

8b15800

Apply review suggestion Co-authored-by: Adam Bucior <35536269+AdamBucior@users.noreply.github.com>

Add allocator_traits and allocator tests

a16d3cc

Tests check if allocation tasks can be completed at compile time.

Enforce clang-format

6cfaf58

Again

Modify allocator_traits:: construct and destroy

3ca91cf

This commit assumes a3e3dc8 is merged first

Modify tests to fix test allocator bugs

d90e6f9

MichaelRizkalla and others added 5 commits December 30, 2020 18:22

Add more test coverage and missing includes

19f0558

clang-format

c82d160

Update tests

cf86b05

- Removing tests that are wrongly written

Revert last 3 commits and add missing include file.

8446779

Change macro contraint of _Default_allocator_traits members

7457abb

mnatsuhara reviewed Jan 6, 2021

View reviewed changes

stl/inc/xmemory Outdated Show resolved Hide resolved

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

Changes to achieve consistency

ad25f39

mnatsuhara approved these changes Jan 6, 2021

View reviewed changes

Merge branch 'master' into MichaelRizkalla/master

08bf68f

StephanTLavavej assigned StephanTLavavej and unassigned mnatsuhara Jan 7, 2021

StephanTLavavej linked an issue Jan 7, 2021 that may be closed by this pull request

P0784R7 Library Support For More constexpr Containers #37

Closed

StephanTLavavej reviewed Jan 7, 2021

View reviewed changes

StephanTLavavej added 4 commits January 6, 2021 21:17

Drop std qualification.

0705f3f

Code review feedback.

d86dbd8

Avoid verbose auto.

a2bd52f

Adjust compiler guards.

bf1c2ec

StephanTLavavej approved these changes Jan 7, 2021

View reviewed changes

StephanTLavavej removed their assignment Jan 7, 2021

miscco reviewed Jan 7, 2021

View reviewed changes

stl/inc/xmemory Outdated Show resolved Hide resolved

miscco reviewed Jan 7, 2021

View reviewed changes

stl/inc/xmemory Outdated Show resolved Hide resolved

Fix compiler guards, add comments.

4ccb165

StephanTLavavej self-assigned this Jan 7, 2021

mnatsuhara approved these changes Jan 8, 2021

View reviewed changes

StephanTLavavej merged commit 2561974 into microsoft:master Jan 8, 2021

mnatsuhara mentioned this pull request Jan 11, 2021

Tracking Issue for constexpr Containers: P0784, P0980, P1004 #1566

Open

MichaelRizkalla deleted the master branch January 28, 2021 09:51

<xmemory> Add constexpr to allocator traits #1369

<xmemory> Add constexpr to allocator traits #1369

Uh oh!

Conversation

MichaelRizkalla commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

ghost commented Oct 14, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco commented Oct 14, 2020

Uh oh!

miscco commented Oct 14, 2020

Uh oh!

MichaelRizkalla commented Oct 14, 2020

Uh oh!

miscco commented Oct 14, 2020

Uh oh!

Uh oh!

Uh oh!

mnatsuhara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

miscco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mnatsuhara left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mnatsuhara left a comment

Choose a reason for hiding this comment

Uh oh!

mnatsuhara commented Jan 6, 2021

Uh oh!

StephanTLavavej left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaelRizkalla commented Oct 14, 2020 •

edited

Loading

ghost commented Oct 14, 2020 •

edited by ghost

Loading