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 compare operators to nostd::string_view #124

Merged
merged 5 commits into from
Jun 29, 2020

Conversation

maxgolov
Copy link
Contributor

This change adds missing methods that exist on standard std::string_view , allowing to use nostd::string_view as key in std::map and other containers.

@maxgolov maxgolov requested review from a team and pyohannes June 22, 2020 21:59
@codecov
Copy link

codecov bot commented Jun 22, 2020

Codecov Report

Merging #124 into master will increase coverage by 0.04%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #124      +/-   ##
==========================================
+ Coverage   93.45%   93.50%   +0.04%     
==========================================
  Files          71       71              
  Lines        1697     1724      +27     
==========================================
+ Hits         1586     1612      +26     
- Misses        111      112       +1     
Impacted Files Coverage Δ
api/include/opentelemetry/nostd/string_view.h 91.89% <93.33%> (-0.42%) ⬇️
api/test/nostd/string_view_test.cc 100.00% <100.00%> (ø)

Copy link
Contributor

@g-easy g-easy left a comment

Choose a reason for hiding this comment

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

Please also api/test/nostd/string_view_test.cc

@maxgolov
Copy link
Contributor Author

Please also api/test/nostd/string_view_test.cc

Yes, I will add the tests.

@kmanghat
Copy link
Contributor

Could we also add hash functions for nostd::string_view that make it usable with unordered_map and unordered_set? I can do this in a separate PR or it could be added in this one.

@maxgolov
Copy link
Contributor Author

Could we also add hash functions for nostd::string_view

Are these part of standard std::string_view ? I'd like to ensure that our implementations of nostd classes are as close to standard library as possible. Why? Because that keeps an option on a table to substitute one for another in a neat way. I was planning to submit a separate PR that validates our nostd classes behavior side-by-side with Standard Library, C++17, C++20, as well as gsl::span. Abseil is another good candidate. Comparing just the bare minimum of classes we implement, and the methods we use in SDK.

@maxgolov
Copy link
Contributor Author

Do you mean something like nostd::hash<nostd::string_view> compatible with std::hash ?

@kmanghat
Copy link
Contributor

Yeah it looks like it is available in c++ 20, yeah I was thinking of something like that. You could also modify std::hash that way you don't have to do something like this std::unordered_set<nostd::string_view, nostd::MyHash> set

@maxgolov maxgolov requested review from g-easy and reyang June 29, 2020 21:58
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM. Please rebase.

@reyang reyang merged commit aa79d9c into open-telemetry:master Jun 29, 2020
nadiaciobanu pushed a commit to nadiaciobanu/opentelemetry-cpp that referenced this pull request Jul 2, 2020
* Add compare operators to nostd::string_view

* Added tests
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.

4 participants