This is an archive of the discontinued LLVM Phabricator instance.

Implement uses-allocator (P0591R4)
AbandonedPublic

Authored by ldionne on Mar 2 2019, 3:18 PM.

Details

Summary

Add uses_allocator_construction_args overloads, make_obj_using_allocator and uninitialized_construct_using_allocator.

Diff Detail

Event Timeline

zoecarver created this revision.Mar 2 2019, 3:18 PM

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.

zoecarver updated this revision to Diff 189063.Mar 2 2019, 3:23 PM

Check off in status

zoecarver updated this revision to Diff 189064.Mar 2 2019, 3:29 PM

Fix trailing white space.

  • Completely re-write this patch.
  • Add tests
  • Fix tuple issues
zoecarver marked 3 inline comments as done.Apr 29 2019, 12:11 PM

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

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

zoecarver updated this revision to Diff 198151.May 4 2019, 5:19 PM
  • 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.

zoecarver updated this revision to Diff 198213.May 5 2019, 7:19 PM
  • finally finish implementing the paper
  • remove construct methods from polymorphic_allocator
zoecarver marked 2 inline comments as done.May 5 2019, 7:25 PM
zoecarver added inline comments.
include/experimental/memory_resource
203 ↗(On Diff #198213)

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

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.

Quuxplusone added inline comments.
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
515

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.

zoecarver marked 4 inline comments as done.May 6 2019, 3:44 PM
zoecarver added inline comments.
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
515

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.

Quuxplusone added inline comments.May 6 2019, 3:59 PM
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.)

zoecarver updated this revision to Diff 199196.May 13 2019, 7:45 AM
zoecarver marked 3 inline comments as done.
  • 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.

zoecarver marked an inline comment as done.May 13 2019, 7:48 AM
zoecarver added inline comments.
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp
122 ↗(On Diff #199196)

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.

zoecarver marked an inline comment as done.May 14 2019, 9:32 AM
zoecarver added inline comments.
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp
122 ↗(On Diff #199196)

I think 2975 might fix this actually.

zoecarver marked an inline comment as done.Jun 6 2019, 8:06 PM
zoecarver added inline comments.
test/std/experimental/memory/memory.polymorphic.allocator.class/memory.polymorphic.allocator.mem/construct_pair_const_lvalue_pair.pass.cpp
122 ↗(On Diff #199196)

This is an experimental library! The patch should not touch it.

zoecarver planned changes to this revision.Jun 6 2019, 8:08 PM
Quuxplusone added inline comments.Jun 7 2019, 9:54 AM
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.
Rule of thumb, every function call needs a _VSTD:: to prevent ADL for _some_ reason or other. :)

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?

zoecarver marked an inline comment as done.Jun 7 2019, 5:20 PM
zoecarver added inline comments.
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).

zoecarver marked an inline comment as done.Jun 8 2019, 9:08 AM
zoecarver added inline comments.
include/memory
5820

Do you mean __uses_allocator_construction_args_impl or is_pair?

Please make sure that you implement LWG 3185 as part of this patch

Please make sure that you implement LWG 3185 as part of this patch

Will do. Thanks.

ldionne commandeered this revision.Sep 19 2023, 9:09 AM
ldionne edited reviewers, added: zoecarver; removed: ldionne.

[Github PR transition cleanup]

This was done in D131898.

Herald added a project: Restricted Project. · View Herald TranscriptSep 19 2023, 9:09 AM
ldionne abandoned this revision.Sep 19 2023, 9:09 AM