You probably want to std::move() the "expensive" parameteres (row_set, filter).

done

You return a RowReader but the class is defined in bigtable/client/internal and presumably in the internal namespace?

done

I think RowReader is part of the interface exposed to the user. They may need to define variables of this type:

bigtable::RowReader reader = table.ReadRows(...);

it cannot be hidden in the internal directory or namespace. We want to be able to say "do not use anything in internal/ or bigtable::internal directly".

Thanks for the timely review! I still owe the tests, but replying to the comments:

done - moved

"Iterate over the results of ReadRows() using the STL idioms"?

changed

constexpr maybe?

done

I think you are required to implement the postfix ++ operator too, though it might be a bad idea to use it.

thanks, I followed your advice and refactored to hold the row in the iterator.

I kept to the absl::optional to be able to use emplace.

Updated the * and -> operators as well by basically snatching the ones from absl::optional.

Doesn't the constant pointer to RowReader disable copying for the iterator class? And I thought iterators were required to be CopyConstructible?

Please consider adding a test where you verify the compile time assertions about iterators:

  // RowReader::iterator must satisfy the requirements of an InputIterator.
  static void check_iterator_requirements() {
    using base_iterator = std::iterator<std::input_iterator_tag, CellReader>;
    static_assert(std::is_base_of<base_iterator, iterator>::value,
                        "iterator must be an InputIterator"); 
    static_assert(std::is_copy_constructible<iterator>::value,
                         "iterator must be CopyConstructible");
    static_assert(std::is_move_constructible<iterator>::value,
                         "iterator must be MoveConstructible");
    static_assert(std::is_copy_assignable<iterator>::value,
                         "iterator must be CopyAssignable");
    static_assert(std::is_move_assignable<iterator>::value,
                         "iterator must be MoveAssignable");
    static_assert(std::is_destructible<iterator>::value,
                         "iterator must be Destructible");
    static_assert(std::is_swappable<iterator>::value,
                         "iterator must be Swappable");
    // ugly hacks to check that if o is of type iterator then ++o and *o 
    // are valid expressions with the usual return types ...
  }

Since these are static asserts, I've put them at the top of row_reader.cc in an anonymous namespace. They did fail until I removed the const field.

I think if you hold the already parse and read Row object in the iterator you can implement postfix, make it copy constructible, etc.

done, thanks

This ties the lifecycle of the RowReader to the lifecycle of the Table used to create it. Consider making a copy.

done

Alternatively, you can have the end() iterators have owner_ == nullptr, and move the Row to the actual iterators. That would make them behave like other iterators do.

done

Thanks for implementing these checks. Can you review the requirements for an InputIterator in this doc, or any documentation you like really, and make sure we are not missing anything?

I think we are exercising the equality and non-equality in tests through assertions. I've also added an explicit use of the postincrement in ReadOneRowIteratorPostincrement.

You do not copy or use retry_policy or backoff_policy. That is because this is WIP and you want to make the interface complete?
If we are going to merge this then we either need a (a) TODO saying why these are unused, or (b) remove them and put the TODO() from wherever this is called.

Oh, I see below that you are using backoff_policy_, you probably want to transfer those policies then.

thanks, fixed

It is hard to figure out what is going on here. Seems like you move the data into response_ and then immediately move it out. Why?

I only move out one chunk at a time. I chose to simplify the parser (compared to what it used to do) and have it take single chunks. Therefore RowReader needs to handle multi-chunk responses, which is what happens here and in NextChunk(). If you agree with the approach, I'll add some comments explaining it.

Ahhh, processed_chunks_ is the count of processed chunks not the vector or list of processed chunks. Do you mind naming that member variable processed_chunks_count_?

With that I think no additional comments would be needed.

done

I personally prefer to minimize if/else depth:

if (blah) {
  do thing;
  continue;
}
do other things...

over:

if (blah) {
  do thing;
} else {
  do other things;
}
// end of loop.

ok, I've rearranged the whole thing a bit

Same preference for shallower if/else here.

done

No blanks between the @param sections?

done

We could use two overloads instead of a magic value... Not sure what is best, I just dislike magic values.

Zero gets special treatment in the protocol, but I agree we can protect the users from accidentally passing it. I've added an overload and enforced that rows_limit, if given, is strictly greater than 0.

This TODO was meant as a "when C++17 is available we can check that iterator works with std::swap() easily". You can probably remove it, assuming the iterator does work with std::swap?

Do you know the standard tricks to check such things? For example, we could do:

static_assert(
  std::is_convertible<decltype(*std::declval<RowReader::iterator>()), RowReader::value_type>::value,
  "*it when it is of RowReader::iterator type must be convertible to RowReader::value_type>);

You can do something similar for the prefix ++.

thanks! done, I think

I have been thinking for std::istream_iterator for inspiration on this. Your implementation seems like a modern version of the libstdc++ one:

https://gcc.gnu.org/onlinedocs/libstdc++/libstdc++-html-USERS-3.4/classstd_1_1istream__iterator.html

I think that is pretty cool.

Is this needed?

done

You are opening and closing the namespace in the same line, are you sure that is what you wanted?

oops, that was not supposed to close there. fixed

This defines an anonymous namespace inside the bigtable namespace, are you sure that is what you wanted?

done

Is this needed?

removed