Make pqxx iterators model the right iterator category

[alexolog]

When using C++20 concepts:

#include <iterator>
#include <print>

#include "pqxx/pqxx"

template<typename T> consteval std::string_view classify()
{
    if constexpr (std::contiguous_iterator<T>)
        return "contiguous_iterator";
    if constexpr (std::random_access_iterator<T>)
        return "random_access_iterator";
    if constexpr (std::bidirectional_iterator<T>)
        return "bidirectional_iterator";
    if constexpr (std::forward_iterator<T>)
        return "forward_iterator";
    if constexpr (std::input_iterator<T>)
        return "input_iterator";
    if constexpr (std::input_or_output_iterator<T>)
        return "input_or_output_iterator";
    return "not an iterator";
}

#define TEST(x) std::println("{} : {}", #x, classify<x>())

int main()
{
    TEST(pqxx::const_row_iterator);
    TEST(pqxx::const_result_iterator);
}

Results in:

pqxx::const_row_iterator : bidirectional_iterator
pqxx::const_result_iterator : bidirectional_iterator

Even though those iterators are "supposed" to model std::random_access_iterator

[jtv]

I thought I did the work for this, sometime long ago.

@alexolog do you know exactly which concepts are missing, and how I implement them?

[jtv]

I think we should be able to get some of the information by trying to instantiate a template that expects a std::random_access_iterator, but passing it a pqxx::const_row_iterator or a pqxx::const_result_iterator. There should be an error, and at least with some compilers, the error will tell us "this type doesn't satisfy the random_access_iterator concept because..."

@alexolog did you see a helpful error like that while compiling your code? (Switching between gcc and clang may help — I think they produce different error messages.

[alexolog]

I thought I did the work for this, sometime long ago.

@alexolog do you know exactly which concepts are missing, and how I implement them?

Looking at LegacyRandomAccessIterator, we can see the requirements:

template< class I >
concept random_access_iterator =
	std::bidirectional_iterator<I> &&
	std::derived_from</*ITER_CONCEPT*/<I>, std::random_access_iterator_tag> &&
	std::totally_ordered<I> &&
	std::sized_sentinel_for<I, I> &&
	requires(I i, const I j, const std::iter_difference_t<I> n) {
		{ i += n } -> std::same_as<I&>;
		{ j +  n } -> std::same_as<I>;
		{ n +  j } -> std::same_as<I>;
		{ i -= n } -> std::same_as<I&>;
		{ j -  n } -> std::same_as<I>;
		{  j[n]  } -> std::same_as<std::iter_reference_t<I>>;
	};

It appears you have most of them, and the only one missing is operator[]. Implement that one and you're golden.

[alexolog]

I think we should be able to get some of the information by trying to instantiate a template that expects a std::random_access_iterator, but passing it a pqxx::const_row_iterator or a pqxx::const_result_iterator. There should be an error, and at least with some compilers, the error will tell us "this type doesn't satisfy the random_access_iterator concept because..."

@alexolog did you see a helpful error like that while compiling your code? (Switching between gcc and clang may help — I think they produce different error messages.

See the example code I included in the original post.
We can find the iterator category by using if constexpr instead of SFINAE.
Both result and row iterator types satisfy bidirectional_iterator and below, but not random_access_iterator or above.

[jtv]

Implementing operator[] isn't so easy. The result iterator already has an operator, but it does something different.

[alexolog]

Implementing operator[] should be easy since

Giving it different semantics (or omitting it altogether) means that the iterator won't be treated as a random-access iterator by libraries (including the standard library), which will, at best, choose to invoke operator++ in a loop instead of the more efficient operator+ (e.g., std::distance), or flatly refuse to accept it in some cases (e.g., std::flat_set<> or std::sort)

This is of course a valid design choice, but you cannot have your cake and eat it too. So the options are:

1. (re)implement operator[] with the expected semantics to have the iterators model std::random_access_iterator, or
2. declare the iterators as bidirectional (using iterator_category = std::bidirectional_iterator_tag;) and document them as such.

(Issue renamed accordingly)

[jtv]

@alexolog for some reason I think some conversation disappeared. But you're right — I wasn't aware of the operator[] requirement back when I wrote that code. So the result iterator is going to have to be a bidirectional iterator for now.

We'll probably need a deprecation period for the existing operator[] in the result iterator — which means we can't fully resolve this in 8.0. :-( On the one hand I see why the concept has that requirement (given how the classic C definition of the operator is just a combination of * and +). On the other hand it really gets in the way of the "referential transparency" that made so much sense in a two-dimensional iteration.

OTOH the row iterator is easy — that didn't have an operator[]. So in the short term I'm thinking...

1. Degradate the result iterators to "bidirectional."
2. Give the row iterators an operator[].

[jtv]

We should be able to resolve this together with #770. Rewrite the iterator types so that they're no longer closely intertwined with the row/field types; give row and const_result_iterator different operator[]s with different return types; and also make row and field keep their result object alive but no longer make the result/row iterators keep their result alive.

[jtv]

Tempting: it now looks like a "divorce" between result::const_iterator::operator[] and row::operator[] probably wouldn't break a lot of code!

I tried commenting out the operator[] on the iterator, and the only code that failed to compile was in tests specifically meant to exercise this kind of thing. The "natural" code I'd written in other tests remained unaffected.

So loath as I am to break compatibility like this, I probably could implement random_access_iterator in libpqxx 8.0.

(For now though, going with the demotion to bidirectional_iterator.)