Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

P0458R2 Checking for existence of an element in associative containers #2163

Merged
merged 1 commit into from Jun 21, 2018

Conversation

jwakely
Copy link
Member

@jwakely jwakely commented Jun 11, 2018

Fixes #2133

@jensmaurer jensmaurer added this to the post-2018-06 milestone Jun 12, 2018
@jensmaurer jensmaurer changed the title P0458R2 Checking for Existence of an Element in Associative Containers P0458R2 Checking for existence of an element in associative containers Jun 15, 2018
@jensmaurer jensmaurer self-requested a review June 15, 2018 13:07
\tcode{con\-tains(ke)} &
\tcode{bool} &
equivalent to \tcode{a_tran.find(ke) != a_tran.end()} &
logarithmic \\ \rowsep
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can do without the a_tran changes; since a_tran is a restriction of "b" and the semantics don't differ, we should be good. (The semantics of find() do differ, though, but that's hidden behind the function call.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should it b b.contains(ke)? Because otherwise we're only defining what happens if you call contains with the key type, and the transparent versions accept anything that can be compared to the key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's the point.
Tell me more: We have these template <class K> overloads all over the place; does that mean they're not active for non-transparent comparators?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But b.contains(ke) doesn't necessarily work, it only works if b has a transparent comparison function, in which case we'd use a_tran.

I think we need both rows. The b row is always valid, the a_tran row (which isn't restricted to arguments convertible to key_type) is only conditionally valid.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that mean they're not active for non-transparent comparators?

Yes, see [associative.reqmts] p14.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so we need to edit p14 to add contains!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After informing LWG of my intention, I've added contains to p14 in a Fixup push to the branch.

@@ -7536,6 +7567,7 @@
iterator find(const key_type& k);
const_iterator find(const key_type& k) const;
size_type count(const key_type& k) const;
bool contains(const key_type& x) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key_type parameter name is called "k" in the vicinity. We should stay consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -8064,6 +8096,7 @@
iterator find(const key_type& k);
const_iterator find(const key_type& k) const;
size_type count(const key_type& k) const;
bool contains(const key_type& x) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key_type parameter name is called "k" in the vicinity. We should stay consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -8390,6 +8423,7 @@
iterator find(const key_type& k);
const_iterator find(const key_type& k) const;
size_type count(const key_type& k) const;
bool contains(const key_type& x) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key_type parameter name is called "k" in the vicinity. We should stay consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -8678,6 +8712,7 @@
iterator find(const key_type& k);
const_iterator find(const key_type& k) const;
size_type count(const key_type& k) const;
bool contains(const key_type& x) const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key_type parameter name is called "k" in the vicinity. We should stay consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks.

[associative.reqmts] Add "contains" to the list of member function
templates that do not participate in overload resolution unless the
comparator is transparent, as agreed on the lib reflector.
@zygoloid zygoloid merged commit 8702000 into master Jun 21, 2018
@jwakely jwakely deleted the motions-2018-06-lwg-16 branch July 6, 2018 23:30
@jwakely jwakely restored the motions-2018-06-lwg-16 branch July 6, 2018 23:31
@jensmaurer jensmaurer deleted the motions-2018-06-lwg-16 branch October 19, 2019 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants