This is an archive of the discontinued LLVM Phabricator instance.

Remove default parameters (P0935R0)
AbandonedPublic

Authored by zoecarver on Apr 8 2019, 2:52 PM.

Details

Summary

Implement changes specified in P0935R0.

There were a few constructors in the queue header which did not look right, so I updated them using my best judgment.

Edit: also, I used _LIBCPP_INLINE_VISIBILITY where it looked like other's were using it. What is the rule of thumb for _LIBCPP_INLINE_VISIBILITY?

Edit 2: I didn't write any tests for this patch because I thought it wasn't needed (and because that would take a lot of time :P), but if you think it would be a good idea I can test for constructors blocking copy-list-initialization (or other things if necessary).

Diff Detail

Event Timeline

zoecarver created this revision.Apr 8 2019, 2:52 PM
zoecarver edited the summary of this revision. (Show Details)Apr 8 2019, 2:54 PM
zoecarver edited the summary of this revision. (Show Details)Apr 8 2019, 2:58 PM
zoecarver updated this revision to Diff 194309.Apr 9 2019, 6:37 AM

Small fixes after running tests

ldionne requested changes to this revision.Apr 9 2019, 6:44 AM

Also, I used _LIBCPP_INLINE_VISIBILITY where it looked like other's were using it. What is the rule of thumb for _LIBCPP_INLINE_VISIBILITY?

Use it for functions that you don't want to be part of the ABI. In this case, every default constructor that simply forwards to another constructor should be marked with _LIBCPP_INLINE_VISIBILITY.

include/queue
42

We normally don't show the definition of functions in the synopsis. Also notice that this default constructor already appears above.

This revision now requires changes to proceed.Apr 9 2019, 6:44 AM
zoecarver marked an inline comment as done.Apr 9 2019, 6:49 AM
zoecarver added inline comments.
include/queue
42

Your right, guess I didn't see that. I will get rid of the added constructor. For the below constructors, would it be helpful to keep them in the synopsis because they show what the default value is?

Use it for functions that you don't want to be part of the ABI. In this case, every default constructor that simply forwards to another constructor should be marked with _LIBCPP_INLINE_VISIBILITY.

Great, thanks for the explanation. I will update the rest of the (forwarding) constructors to use _LIBCPP_INLINE_VISIBILITY.

ldionne added inline comments.Apr 9 2019, 6:56 AM
include/queue
42

I think it's OK to keep the definition for the other constructors since they're one-liners, yes.

zoecarver updated this revision to Diff 194428.Apr 9 2019, 5:44 PM
zoecarver marked 2 inline comments as done.

Fix synopsis and update www.

EricWF added a comment.Apr 9 2019, 7:08 PM

Every line of this change should be covered by a test

include/queue
452

copy paste error? we are copy constructing the comparator

lib/CMakeLists.txt
202 ↗(On Diff #194428)

bad diff?

zoecarver marked 2 inline comments as done.Apr 9 2019, 8:01 PM

Sounds good. I will work on tests for these on a plane ride I have Sunday.

include/queue
452

Yes, my bad. I should get rid of this and keep line 470-472.

lib/CMakeLists.txt
202 ↗(On Diff #194428)

Yep, forgot to pull from master first. Sorry.

EricWF requested changes to this revision.Apr 27 2019, 10:22 PM

Every line of this change should be covered by a test.

For each type, we need at least one new test that checks that the default constructor can be called implicitly.
( You can use test/support/test_convertible.hpp for this)

And if we don't have an existing test that the other constructors are explicit, we should add one.

The tests should be added alongside the existing tests for the constructors. Let me know if you need help finding them.

This revision now requires changes to proceed.Apr 27 2019, 10:22 PM
zoecarver updated this revision to Diff 197697.May 1 2019, 8:18 PM
  • "starter" test
  • fix diff
zoecarver marked 2 inline comments as done.May 1 2019, 8:23 PM
zoecarver added inline comments.
include/queue
229

I didn't see this in the standard, but I can keep it.

test/std/containers/container.adaptors/queue/queue.defn/default_constructor.pass.cpp
1

Before I spend a lot of time writing tests for this, I want to check that I am writing the correct tests. I am going to use this test as a "template" for other tests. @EricWF how does this look?

ldionne requested changes to this revision.May 12 2020, 8:28 AM

It's been a while, but I'll review this if you update it with my comments.

test/std/containers/container.adaptors/queue/queue.defn/default_constructor.pass.cpp
1

IIUC, you're just trying to test that the type is implicitly default constructible. You could add a support header that contains generic tests for that, like test_implicitly_default_constructible<T>(), and then you'd simply use test_implicitly_default_constructible<std::queue<T>>(), etc.

This revision now requires changes to proceed.May 12 2020, 8:28 AM
zoecarver marked an inline comment as done.May 12 2020, 7:42 PM

It's been a while, but I'll review this if you update it with my comments.

Sounds good to me. I might not have time to work on this patch until this weekend or next week, though.

test/std/containers/container.adaptors/queue/queue.defn/default_constructor.pass.cpp
1

I like that idea a lot. I'll do that.

zoecarver abandoned this revision.Nov 19 2020, 11:40 PM

Abandoning in favor of D91292.