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

Add a cpl::contains(container, value) helper #10464

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Conversation

rouault
Copy link
Member

@rouault rouault commented Jul 21, 2024

and use it in gcore

@dbaston Thoughts? I'm quite tired of writing code like container.find(key) != containers.end(). As of C++17, we could use container.count(key), as the closest alternative to C++20 container.contains(key), but using count() might complicate future modernization when we bump to C++20. This cpl::contains(container, value) should be easily greppable to automate a change to container.contains(value)

@coveralls
Copy link
Collaborator

Coverage Status

coverage: 69.298% (-0.003%) from 69.301%
when pulling 93ec1b7 on rouault:cpl_contains
into b6b5546 on OSGeo:master.

gcore/gdaldriver.cpp Show resolved Hide resolved
@rouault rouault merged commit 3122857 into OSGeo:master Jul 23, 2024
35 checks passed
@rouault rouault modified the milestones: 3.9.2, 3.10.0 Jul 23, 2024
template <typename C, typename V>
inline bool contains(const C &container, const V &value)
{
return container.count(value) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, doesn't count always need to do too much? It cannot stop at the first match.

Copy link
Member Author

Choose a reason for hiding this comment

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

not for std::set and std::map, which are up to now the only users of that

Copy link
Contributor

Choose a reason for hiding this comment

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

not for std::set and std::map

... unless some call triggers the second overload of count? (Taking an "equivalent" key instead of an exact one.)

up to now the only users of that

This is a very generic function interface, but the implementation is specific. It gets attention (read: review) when added or modified. It is unlikely to get attention when a different use is added elsewhere. Including in software using GDAL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for pointing me to this new C++14 second overload / "equivalent" key concept. I have a bit of a hard time fully understanding the impact of that , but my understanding of https://stackoverflow.com/questions/20317413/what-are-transparent-comparators, is that it is only available if you define something like a std::key or std::map specially constructed with a std::less<>, so that shouldn't occur with the current state of the code base. That said in #10518 I'm proposing to use a std::find() based implementation to avoid any performance issue.

It is unlikely to get attention when a different use is added elsewhere. Including in software using GDAL.

This can't as this is protected by a ifdef GDAL_COMPILATION. So for GDAL internal use only

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.

5 participants