Add uses_allocator_construction_args overloads, make_obj_using_allocator and uninitialized_construct_using_allocator.
Details
Diff Detail
Event Timeline
The paper also requests the deletion of several construct methods. I did not add this to the patch because it broke a lot of tests (and other things). I thought it would be worth asking if we should keep them for compatibility reasons.
Because of the length of the name uses_allocator_construction_args, some of the styles are... different. Feel free to suggest changes.
include/memory | ||
---|---|---|
5710 | Spacing | |
include/tuple | ||
1377 ↗ | (On Diff #197159) | I can submit a separate patch for this. |
test/std/utilities/memory/allocator.uses/allocator.uses.construction/uses_allocator_construction_args.pass.cpp | ||
10 | Add synopsis comment |
- Finishes implementing the paper (remove scoped_allocator::construct)
- Updates and adds tests
I think P0475R1 can also be marked complete because the code it refers to has been removed.
- finally finish implementing the paper
- remove construct methods from polymorphic_allocator
include/experimental/memory_resource | ||
---|---|---|
203 | Please note: this is not tested (and probably needs further changes). Because of 41760 (which I am starting to think is my fault and not an actual bug), I was unable to run many of the polymorphic_allocator tests and therefore could not ensure that this was correctly implemented. | |
include/tuple | ||
1377 ↗ | (On Diff #197159) | Given that a fix for this goes against the standard, I am not sure what to do here. First, I am going to look into this more and make sure it is necessary. As a note, the change here is not my suggested fix. Please see D61293 for that. |
include/memory | ||
---|---|---|
5751 | What is -> auto doing here and on line 5755? | |
5798 | So if I pass in a pair<string, int> or pair<string&&, int>, you'll move out of p.first, but if I pass in pair<string&, int>, you'll make a copy of p.first, is that right? I guess that seems reasonable, but it's a surprising/smelly use of std::forward IMVHO. | |
include/scoped_allocator | ||
513 | This function body is indented an extra level. Not your patch's fault, but I wonder if your patch should fix it. | |
test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp | ||
49 | What's the rationale for all these test changes? I strongly suspect that they are a bad idea: if the original tests fail after your patch, I think there must be something wrong with your patch. |
include/memory | ||
---|---|---|
5751 | Will fix. | |
5798 | You are right; I should probably use std::move here. For reference here is what the standard says. | |
include/scoped_allocator | ||
513 | Yep, good catch. I can fix that. | |
test/std/utilities/allocator.adaptor/allocator.adaptor.members/construct_pair.pass.cpp | ||
49 | These are all simply adding const or changing the reference type (I am not saying that necessarily makes them okay though). Because of how uses_allocator_construction_args is defined in the standard, make_tuple is passed an rvalue piecewise_construct (that is why the first part of this changed). I think the others are similar. I will take a closer look at this and see which test is correct. |
include/memory | ||
---|---|---|
5798 | Thanks for pointing to the relevant Standard wording! I was too lazy to go look for it myself. :) The wording uses std::move(pr).first, which is equivalent to std::forward<_T1>(pr.first) but doesn't set off my red-flag detectors — I would greatly prefer _VSTD::move(__p).first here. (Notice that _VSTD::move(__p).first is different from _VSTD::move(__p.first) when __p is a pair of lvalue references. Which is good; it's what we want.) |
- disable experimental tests
- update other tests
I removed the temporary "fix" I added to make_from_tuple because it was causing other failures and I wanted to discuss it before implementing something. The issue has to do with the move vs forward thread below. Using std::move(pair).first changes rvalues into lvalues and vice versa whereas forward consistently makes everything an rvalue (in the previous implementation, obviously not always). This clearly creates issues and is the reason that the tests are failing.
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp | ||
---|---|---|
122 | I really don't like disabling tests, but I think that these tests are incorrect (at least for c++20). Uses-allocator construction requires that types (or pair types) implement a uses-allocator typedef that is the same as the allocator they are being passed. This means that when these tests are passed the wrong allocator type, they choose the incorrect (well... actually it is correct) overload (the first one). If I am mistaken about this, let me know, and I will change the uses-allocator implementation. |
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp | ||
---|---|---|
122 | This is an experimental library! The patch should not touch it. |
include/memory | ||
---|---|---|
5820 | Nit: I believe this needs a _VSTD:: to prevent ADL in some complicated case involving std::pair<int, std::vector<Incomplete>*> that I don't 100% remember. (Something like it's going to try to look for friends of Incomplete because Incomplete is an associated entity of that pair type.) It would be interesting to write a unit test for this. |
As I have worked through fixing all the issues around this patch, I think I have come to the two root problems. First, using forward_as_tuple makes it so that the tuple cannot be copied (rightfully so) and are therefore useless in make_from_tuple. Here is an example. This should probably be fixed with an LWG issue (either to change what tuples can be copied or update make_from_tuple). Second, and more important, the expression forward_as_tuple(move(p).second) is unable to preserve rvalueness of a type. Here is an example. I think the paper either needs to be updated or an issue needs to be presented to fix this (I am happy to write that issue). What are your thoughts @mclow.lists, @EricWF, and @Quuxplusone?
include/memory | ||
---|---|---|
5798 | I am not sure that is necessarily true. Sometimes one works, and the other doesn't. I think it should be pointed out that the author of the paper used forward in their implementation (probably for that reason). |
include/memory | ||
---|---|---|
5820 | Do you mean __uses_allocator_construction_args_impl or is_pair? |
include/memory | ||
---|---|---|
5840 | Should make_obj_using_allocator be qualified? |
Please note: this is not tested (and probably needs further changes). Because of 41760 (which I am starting to think is my fault and not an actual bug), I was unable to run many of the polymorphic_allocator tests and therefore could not ensure that this was correctly implemented.