Page MenuHomePhabricator

Partially implement C++20's P1020/P1973.
Needs ReviewPublic

Authored by ckennelly on Oct 24 2020, 6:54 PM.

Details

Reviewers
EricWF
mvels
ldionne
Group Reviewers
Restricted Project
Summary

This commit adds std::make_unique_for_overwrite.

Diff Detail

Event Timeline

ckennelly created this revision.Oct 24 2020, 6:54 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 24 2020, 6:54 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ckennelly requested review of this revision.Oct 24 2020, 6:54 PM
ckennelly updated this revision to Diff 300521.Oct 24 2020, 8:08 PM

Fixing lint errors

ckennelly updated this revision to Diff 300522.Oct 24 2020, 8:18 PM
  • Fix synposis
  • Restrict tests to C++20
ldionne requested changes to this revision.Oct 26 2020, 9:57 AM
ldionne added a subscriber: ldionne.

Thanks a lot for the patch! Do you think you could update the patch with the rest of the paper too? I'm fine reviewing it all in one patch (actually I find it easier to validate that the whole paper is implemented that way).

Also, I don't see any feature test macro in the paper -- can you confirm that's the case, and that we're not forgetting to add one?

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.array2.compile.fail.cpp
10–11 ↗(On Diff #300522)

You don't need these includes (here and elsewhere).

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.array3.compile.fail.cpp
14 ↗(On Diff #300522)

Can you make these tests be .verify.cpp instead, and check for the error message?

This revision now requires changes to proceed.Oct 26 2020, 9:57 AM

Thanks a lot for the patch! Do you think you could update the patch with the rest of the paper too? I'm fine reviewing it all in one patch (actually I find it easier to validate that the whole paper is implemented that way).

I looked at it, but I realized I needed to implement P0674R1 as a prereq for the make_shared_for_overwrite array bits, which looked more involved than trying to implement all 3 papers in one go

Also, I don't see any feature test macro in the paper -- can you confirm that's the case, and that we're not forgetting to add one?

There is a feature test macro, but because I did not implement the make_shared_for_overwrite parts

ckennelly updated this revision to Diff 300865.Oct 26 2020, 7:55 PM

Apply feedback on review

  • Remove unused includes
  • Use .verify.cpp to check expected error
mvels added a comment.Oct 27 2020, 3:34 AM

Sorry, didn't see this earlier as this drowned in my email filters :/

Arrays in std::shared_ptr is https://reviews.llvm.org/D62641. I've been meaning to review it forever but never get the time to get into it fully. If you'd like, please consider taking a first pass at it.

I'm personally fine taking small bits of a paper.
I would rather it than have a full implementation get stuck in review.

This patch LGTM apart from the negative compilation tests. I suspect those can be SFINAE tests instead.

libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.create/make_unique_for_overwrite.array2.compile.verify.cpp
13

Can we not write positive compilation tests for this? Using things like is_invocable?

Negative compilation tests should be the last resort.

After talking to @ldionne, the negative compilation tests aren't inappropriate here. I'm happy to see the stay.

ckennelly updated this revision to Diff 301159.Oct 27 2020, 6:31 PM

Restrict verify tests to C++20

We do not expose std::make_unique_for_overwrite on pre-C++20 builds, so we will
not get the expected error message.

The test failures look like unrelated tests with flakes.

@ldionne: Do you want to get patch for P0674R1 reviewed first and I can turn this into a complete implementation of P1020/P1973 or should we land the partial implementation?

Ping

We've been working hard to fix some underlying issues in std::shared_ptr first. We'll do that, and then get to this patch. It's on my radar, don't worry, it's just that the mess we needed to fix before we get to this ended up being larger than expected.