You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "jmalkin (via GitHub)" <gi...@apache.org> on 2023/02/01 17:06:23 UTC
[GitHub] [datasketches-cpp] jmalkin opened a new pull request, #342: Remove inheritance from std::iterator
jmalkin opened a new pull request, #342:
URL: https://github.com/apache/datasketches-cpp/pull/342
Not entirely confident on all the difference types, and I punted on the pointer type to a std::pair<> we return since I don't think it'll matter with how we use things.
Anyway, I don't expect this to be the final version but this does at least seem to work with both gcc and clang (14) for a few different c++ standards (11 and 20 at least)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin merged pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin merged PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412628370
Addressed the issues identified with the FI hash map iterator
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412639553
how can operator*() not exist?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#discussion_r1093632567
##########
fi/include/reverse_purge_hash_map.hpp:
##########
@@ -107,8 +113,8 @@ class reverse_purge_hash_map<K, V, H, E, A>::iterator: public std::iterator<std:
iterator operator++(int) { iterator tmp(*this); operator++(); return tmp; }
bool operator==(const iterator& rhs) const { return count == rhs.count; }
bool operator!=(const iterator& rhs) const { return count != rhs.count; }
- const std::pair<K&, V> operator*() const {
- return std::pair<K&, V>(map->keys_[index], map->values_[index]);
+ const value_type operator*() const {
Review Comment:
I think this should return reference type (same as value_type)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#discussion_r1093657883
##########
fi/include/reverse_purge_hash_map.hpp:
##########
@@ -107,8 +113,8 @@ class reverse_purge_hash_map<K, V, H, E, A>::iterator: public std::iterator<std:
iterator operator++(int) { iterator tmp(*this); operator++(); return tmp; }
bool operator==(const iterator& rhs) const { return count == rhs.count; }
bool operator!=(const iterator& rhs) const { return count != rhs.count; }
- const std::pair<K&, V> operator*() const {
- return std::pair<K&, V>(map->keys_[index], map->values_[index]);
+ const value_type operator*() const {
+ return value_type(map->keys_[index], map->values_[index]);
Review Comment:
I don't see how the implementation can do anything else? It seems weird to implement as `reference(key, value)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412883023
> You must be talking about operator->
Yeah, that seems to be what I had in mind. It seems to have been missed for one of the sampling iterators so I'll see if I can get that in. Right now my local build has some sort of test failure so I clearly screwed up something.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1413126197
Now I think it _really_ is ready for re-review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412638720
I think this approach with reference = value_type should be used everywhere we return a proxy object (all quantiles)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412634303
So is the suggestion that we should do that everywhere, including defining operator*() where it doesn't currently exist?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412850108
> including defining operator*() where it doesn't currently exist
I you must be talking about operator->
I think I added it everywhere except the reverse purge hash map, which is for internal use
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on a diff in pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#discussion_r1093655433
##########
fi/include/reverse_purge_hash_map.hpp:
##########
@@ -91,8 +91,14 @@ class reverse_purge_hash_map {
// This iterator uses strides based on golden ratio to avoid clustering during merge
template<typename K, typename V, typename H, typename E, typename A>
-class reverse_purge_hash_map<K, V, H, E, A>::iterator: public std::iterator<std::input_iterator_tag, K> {
+class reverse_purge_hash_map<K, V, H, E, A>::iterator {
public:
+ using iterator_category = std::input_iterator_tag;
+ using value_type = std::pair<K&, V>;
+ using difference_type = std::ptrdiff_t;
Review Comment:
Good point
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412576462
https://en.cppreference.com/w/cpp/named_req/InputIterator#Concept
"The reference type for an input iterator that is not also a LegacyForwardIterator does not have to be a reference type: dereferencing an input iterator may return a proxy object or value_type itself by value (as in the case of std::istreambuf_iterator)"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412567066
I think this is not quite right, and we did not do it quite right so far.
operator*() should return reference
but the reference can be defined to be the same as value_type for LegacyInputIterator in such cases when we return a proxy object by value
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#discussion_r1093634170
##########
fi/include/reverse_purge_hash_map.hpp:
##########
@@ -107,8 +113,8 @@ class reverse_purge_hash_map<K, V, H, E, A>::iterator: public std::iterator<std:
iterator operator++(int) { iterator tmp(*this); operator++(); return tmp; }
bool operator==(const iterator& rhs) const { return count == rhs.count; }
bool operator!=(const iterator& rhs) const { return count != rhs.count; }
- const std::pair<K&, V> operator*() const {
- return std::pair<K&, V>(map->keys_[index], map->values_[index]);
+ const value_type operator*() const {
+ return value_type(map->keys_[index], map->values_[index]);
Review Comment:
and instantiating value_type here is fine as implementation, I suppose
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] coveralls commented on pull request #342: Remove inheritance from std::iterator
Posted by "coveralls (via GitHub)" <gi...@apache.org>.
coveralls commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412417647
## Pull Request Test Coverage Report for [Build 4066836102](https://coveralls.io/builds/56513358)
* **2** of **2** **(100.0%)** changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage remained the same at **93.873%**
---
| Totals | [](https://coveralls.io/builds/56513358) |
| :-- | --: |
| Change from base [Build 4049479609](https://coveralls.io/builds/56433514): | 0.0% |
| Covered Lines: | 2329 |
| Relevant Lines: | 2481 |
---
##### 💛 - [Coveralls](https://coveralls.io)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] jmalkin commented on pull request #342: Remove inheritance from std::iterator
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#issuecomment-1412777843
I think this is ready for review pass 2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#discussion_r1093628515
##########
fi/include/reverse_purge_hash_map.hpp:
##########
@@ -91,8 +91,14 @@ class reverse_purge_hash_map {
// This iterator uses strides based on golden ratio to avoid clustering during merge
template<typename K, typename V, typename H, typename E, typename A>
-class reverse_purge_hash_map<K, V, H, E, A>::iterator: public std::iterator<std::input_iterator_tag, K> {
+class reverse_purge_hash_map<K, V, H, E, A>::iterator {
public:
+ using iterator_category = std::input_iterator_tag;
+ using value_type = std::pair<K&, V>;
+ using difference_type = std::ptrdiff_t;
Review Comment:
I think this can be void in this case. I would be much more comfortable not specifying pointer difference for temporary values
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-cpp] AlexanderSaydakov commented on a diff in pull request #342: Remove inheritance from std::iterator
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #342:
URL: https://github.com/apache/datasketches-cpp/pull/342#discussion_r1093629531
##########
fi/include/reverse_purge_hash_map.hpp:
##########
@@ -91,8 +91,14 @@ class reverse_purge_hash_map {
// This iterator uses strides based on golden ratio to avoid clustering during merge
template<typename K, typename V, typename H, typename E, typename A>
-class reverse_purge_hash_map<K, V, H, E, A>::iterator: public std::iterator<std::input_iterator_tag, K> {
+class reverse_purge_hash_map<K, V, H, E, A>::iterator {
public:
+ using iterator_category = std::input_iterator_tag;
+ using value_type = std::pair<K&, V>;
+ using difference_type = std::ptrdiff_t;
+ using pointer = void;
+ using reference = value_type&;
Review Comment:
I think we should make this = value_type
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org