This is an archive of the discontinued LLVM Phabricator instance.

Add contains method to map types
ClosedPublic

Authored by zoecarver on Mar 13 2019, 8:33 PM.

Details

Summary

Implements P0458R2, adding contains to map, multimap, unordered_map, unordered_multimap, set, multiset, unordered_set, and (finally) unordered_multiset.

My editor messed up the spacing when I originally made this patch so let me know if you find any weird white space.

This patch is the predecessor to P0920R2 which I can submit once this one has been reviewed.

Diff Detail

Event Timeline

zoecarver created this revision.Mar 13 2019, 8:33 PM
zoecarver marked 2 inline comments as done.Mar 13 2019, 8:38 PM
zoecarver added inline comments.
include/map
194 ↗(On Diff #190561)

Seems like this should be the correct spacing but I can revert it if you want.

include/set
364 ↗(On Diff #190561)

count isnt in the summary but it is declared in the class.

We don't usually gang tests for multiple containers together - one/container is more common,

include/map
198 ↗(On Diff #190561)

Add a comment to the synopsis indicating that this is a C++20 feature: // C++20

EricWF added a subscriber: EricWF.Mar 13 2019, 11:47 PM
EricWF added inline comments.
test/std/containers/contains.pass.cpp
1 ↗(On Diff #190561)

I really really REALLY like the way you wrote this test!

If a constraint is shared across all containers, or is specified in the container requirements table, then we should test it like this.

ldionne requested changes to this revision.Mar 14 2019, 6:27 AM
ldionne added a reviewer: ldionne.
ldionne added inline comments.
include/unordered_map
1268 ↗(On Diff #190561)

You forgot to add to the synopsis.

test/std/containers/contains.pass.cpp
1 ↗(On Diff #190561)

Yes, I share that feeling.

zoecarver marked an inline comment as done.Mar 14 2019, 7:31 AM
zoecarver added inline comments.
test/std/containers/contains.pass.cpp
1 ↗(On Diff #190561)

Awesome! Happy you like it.

@mclow.lists

We don't usually gang tests for multiple containers together - one/container is more common,

Do you want me to break this into multiple tests?

zoecarver marked 2 inline comments as done.Mar 14 2019, 11:40 AM

Fixed:

  • main args
  • synopsis comment
  • // C++20
  • remove __hash (from other patch)
ldionne requested changes to this revision.Mar 15 2019, 10:31 AM

Please also mark the feature as implemented in www/cxx2a_status.html.

include/set
164 ↗(On Diff #190692)

Please mark all the contains functions added in C++20 as // C++20.

This revision now requires changes to proceed.Mar 15 2019, 10:31 AM
zoecarver updated this revision to Diff 190852.EditedMar 15 2019, 10:43 AM

Fix:

  • add // C++20 to all synopsis comments
  • update test with license and header comments
  • update www/status
ldionne requested changes to this revision.Mar 15 2019, 10:45 AM

This is almost good to go except for a nitpick!

test/std/containers/contains.pass.cpp
8 ↗(On Diff #190852)

We normally put the license first thing in the file.

This revision now requires changes to proceed.Mar 15 2019, 10:45 AM
zoecarver marked an inline comment as done.Mar 15 2019, 10:48 AM
zoecarver added inline comments.
test/std/containers/contains.pass.cpp
8 ↗(On Diff #190852)

So license, then #includes, then other comments, then code?

Fix license comment.

ldionne accepted this revision.Mar 15 2019, 10:59 AM
This revision is now accepted and ready to land.Mar 15 2019, 10:59 AM
jloser added a subscriber: jloser.Apr 4 2019, 7:14 AM
jloser added inline comments.Apr 4 2019, 7:17 AM
include/map
1408 ↗(On Diff #190853)

I think we should only provide the declaration and definition of contains when _LIBCPP_STD_VER > 17. Otherwise, someone compiling with trunk and -std=c++14 would be able to use contains, and this is only meant for C++20 or later.

ldionne requested changes to this revision.Apr 4 2019, 7:50 AM
ldionne added inline comments.
include/map
1408 ↗(On Diff #190853)

Hmm yes, this was the intent but somehow I missed that during the review. Please only enable in C++20 and above.

This revision now requires changes to proceed.Apr 4 2019, 7:50 AM
zoecarver marked an inline comment as done.Apr 4 2019, 7:58 AM
zoecarver added inline comments.
include/map
1408 ↗(On Diff #190853)

Will do.

zoecarver updated this revision to Diff 193829.Apr 4 2019, 7:53 PM

Only enable contains after C++17.

ldionne accepted this revision.Apr 5 2019, 12:13 PM
This revision is now accepted and ready to land.Apr 5 2019, 12:13 PM
jloser added inline comments.Apr 5 2019, 7:44 PM
include/map
2051 ↗(On Diff #193829)

Just a note that while contains supports transparent keys now, find does not yet. Are you planning on implementing http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0919r3.html or want me to do it?

zoecarver marked an inline comment as done.Apr 5 2019, 8:28 PM
zoecarver added inline comments.
include/map
2051 ↗(On Diff #193829)

I already implemented it in D59886. I didn't want to have to wait on one of the patches so I went a bit out of scope by adding transparent keys to this patch.

zoecarver updated this revision to Diff 208310.Jul 7 2019, 8:56 PM
  • rebase off master

Is the mono-test OK? If so, I will commit.

zoecarver updated this revision to Diff 208311.Jul 7 2019, 9:09 PM
  • remove templated contains methods (we will implement those in D59886)
  • split tests up into four files

As soon as this is approved, I will commit. It would be great to get this committed before Wednesday.

My approval still stands, it looked good last time I looked at it and nothing has changed since then. I think you can commit this.

zoecarver closed this revision.Jul 15 2019, 8:24 PM

Committed as rL366170.