fix!: avoid single-arg forwarding c'tor (#1233)

Single-argument forwarding constructors can hide implicitly generated
copy/move constructors. Even if this issue is avoided with the proper
use of SFINAE, clang-tidy may still complain (incorrectly, I think).
http://clang.llvm.org/extra/clang-tidy/checks/bugprone-forwarding-reference-overload.html

To fix this I renamed the class `StreamOf` -> `TupleStream`. I then
changed its constructor to accept to iterators instead of a single
range. Then I added a `StreamOf<T>()` non-member factory function. This
should make it so our existing uses all continue to work.

So, this really shouldn't be much of an API break.

Author: devjgm

google/cloud/spanner/row.h

@@ -372,10 +372,12 @@ class TupleStreamIterator {
 };
 
 /**
- * A `StreamOf<Tuple>` defines a range that parses `Tuple` objects from the
+ * A `TupleStream<Tuple>` defines a range that parses `Tuple` objects from the
  * given range of `RowStreamIterator`s.
  *
- * Users will typically use this class in a range-for loop as follows:
+ * Users create instances using the `StreamOf<T>(range)` non-member factory
+ * function (defined below). The following is a typical usage of this class in
+ * a range-for loop.
  *
  * @code
  * auto row_range = ...
@@ -397,40 +399,45 @@ class TupleStreamIterator {
  * @tparam Tuple the std::tuple<...> to parse each `Row` into.
  */
 template <typename Tuple>
-class StreamOf {
+class TupleStream {
  public:
   using iterator = TupleStreamIterator<Tuple>;
   static_assert(internal::IsTuple<Tuple>::value,
-                "StreamOf<T> requires a std::tuple parameter");
-
-  /**
-   * Creates a `StreamOf<Tuple>` by wrapping the given @p range. The `RowRange`
-   * must be a range defined by `RowStreamIterator` objects.
-   *
-   * @note ownership of the @p range is not transferred, so it must outlive the
-   * `StreamOf`.
-   *
-   * @tparam RowRange must be a range defined by `RowStreamIterator`s.
-   */
-  template <typename RowRange>
-  explicit StreamOf(RowRange&& range)
-      : begin_(std::begin(std::forward<RowRange>(range)),
-               std::end(std::forward<RowRange>(range))) {
-    using T = decltype(std::begin(range));
-    static_assert(std::is_same<RowStreamIterator, T>::value,
-                  "StreamOf must be given a RowStreamIterator range.");
-    static_assert(std::is_lvalue_reference<decltype(range)>::value,
-                  "range must be an lvalue since it must outlive StreamOf");
-  }
+                "TupleStream<T> requires a std::tuple parameter");
 
   iterator begin() const { return begin_; }
   iterator end() const { return end_; }
 
  private:
+  template <typename T, typename RowRange>
+  friend TupleStream<T> StreamOf(RowRange&& range);
+
+  template <typename It>
+  explicit TupleStream(It&& start, It&& end)
+      : begin_(std::forward<It>(start), std::forward<It>(end)) {}
+
   iterator begin_;
   iterator end_;
 };
 
+/**
+ * A factory that creates a `TupleStream<Tuple>` by wrapping the given @p
+ * range. The `RowRange` must be a range defined by `RowStreamIterator`
+ * objects.
+ *
+ * @note ownership of the @p range is not transferred, so it must outlive the
+ *     returned `TupleStream`.
+ *
+ * @tparam RowRange must be a range defined by `RowStreamIterator`s.
+ */
+template <typename Tuple, typename RowRange>
+TupleStream<Tuple> StreamOf(RowRange&& range) {
+  static_assert(std::is_lvalue_reference<decltype(range)>::value,
+                "range must be an lvalue since it must outlive StreamOf");
+  return TupleStream<Tuple>(std::begin(std::forward<RowRange>(range)),
+                            std::end(std::forward<RowRange>(range)));
+}
+
 /**
  * Returns the only row from a range that contains exactly one row.
  *

google/cloud/spanner/row_test.cc

@@ -403,7 +403,7 @@ TEST(TupleStreamIterator, Error) {
   EXPECT_EQ(it, end);
 }
 
-TEST(StreamOf, Basics) {
+TEST(TupleStream, Basics) {
   std::vector<Row> rows;
   rows.emplace_back(MakeTestRow(1, "foo", true));
   rows.emplace_back(MakeTestRow(2, "bar", true));
@@ -438,7 +438,7 @@ TEST(StreamOf, Basics) {
   EXPECT_EQ(it, end);
 }
 
-TEST(StreamOf, RangeForLoop) {
+TEST(TupleStream, RangeForLoop) {
   std::vector<Row> rows;
   rows.emplace_back(MakeTestRow({{"num", Value(2)}}));
   rows.emplace_back(MakeTestRow({{"num", Value(3)}}));
@@ -454,7 +454,7 @@ TEST(StreamOf, RangeForLoop) {
   EXPECT_EQ(product, 30);
 }
 
-TEST(StreamOf, IterationError) {
+TEST(TupleStream, IterationError) {
   std::vector<StatusOr<Row>> rows;
   rows.emplace_back(MakeTestRow(1, "foo", true));
   rows.emplace_back(Status(StatusCode::kUnknown, "some error"));
@@ -463,7 +463,7 @@ TEST(StreamOf, IterationError) {
   RowRange range(MakeRowStreamIteratorSource(rows));
 
   using RowType = std::tuple<std::int64_t, std::string, bool>;
-  StreamOf<RowType> stream(range);
+  auto stream = StreamOf<RowType>(range);
 
   auto end = stream.end();
   auto it = stream.begin();