apache / datasketches-cpp

Core C++ Sketch Library
https://datasketches.apache.org
Apache License 2.0
223 stars 71 forks source link

Remove inheritance from std::iterator #342

Closed jmalkin closed 1 year ago

jmalkin commented 1 year ago

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)

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4071110351


Totals Coverage Status
Change from base Build 4049479609: 0.0%
Covered Lines: 2329
Relevant Lines: 2481

💛 - Coveralls
AlexanderSaydakov commented 1 year ago

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

AlexanderSaydakov commented 1 year ago

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)"

jmalkin commented 1 year ago

Addressed the issues identified with the FI hash map iterator

jmalkin commented 1 year ago

So is the suggestion that we should do that everywhere, including defining operator*() where it doesn't currently exist?

AlexanderSaydakov commented 1 year ago

I think this approach with reference = value_type should be used everywhere we return a proxy object (all quantiles)

AlexanderSaydakov commented 1 year ago

how can operator*() not exist?

jmalkin commented 1 year ago

I think this is ready for review pass 2

AlexanderSaydakov commented 1 year ago

including defining operator*() where it doesn't currently exist

You must be talking about operator-> I think I added it everywhere except the reverse purge hash map, which is for internal use

jmalkin commented 1 year ago

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.

jmalkin commented 1 year ago

Now I think it really is ready for re-review