[base] Allow non-trivial value_types in IsContiguousIterator

This change fixes an issue inside IsContiguousIterator that would fail
compilation if it was instantiated with an iterator to a container that
has a non-trivial value_type.

This was because `IsContiguousIterator<It>` instantiated
`std::basic_string<iter_value_t<It>>` to determine whether `It` was an
iterator for a `std::basic_string`. This would then trigger a
static_assert inside basic_string if `iter_value_t<It>` was not a trivial
type. Now IsContiguousIterator<It> only attempts to instantiate a
std::basic_string if `iter_value_t<It>` is a trivial type, avoiding this
issue.

Bug: 828324
Change-Id: I1ab8d10b1847cca09041cc3f2fe623d2a04602fa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2632958
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844711}

base/containers/contiguous_iterator.h

@@ -26,13 +26,19 @@ struct PointsToObject : std::is_object<iter_value_t<T>> {};
 template <typename T>
 struct IsPointer : std::is_pointer<T> {};
 
+template <typename T, typename StringT = std::basic_string<iter_value_t<T>>>
+struct IsStringIterImpl
+    : disjunction<std::is_same<T, typename StringT::const_iterator>,
+                  std::is_same<T, typename StringT::iterator>> {};
+
 // An iterator to std::basic_string is contiguous.
 // Reference: https://wg21.link/basic.string.general#2
-template <typename T, typename StringT = std::basic_string<iter_value_t<T>>>
+//
+// Note: Requires indirection via `IsStringIterImpl` to avoid triggering a
+// `static_assert(is_trivial_v<value_type>)` inside libc++'s std::basic_string.
+template <typename T>
 struct IsStringIter
-    : conjunction<std::is_trivial<iter_value_t<T>>,
-                  disjunction<std::is_same<T, typename StringT::const_iterator>,
-                              std::is_same<T, typename StringT::iterator>>> {};
+    : conjunction<std::is_trivial<iter_value_t<T>>, IsStringIterImpl<T>> {};
 
 // An iterator to std::array is contiguous.
 // Reference: https://wg21.link/array.overview#1

base/containers/contiguous_iterator_unittest.cc

@@ -111,7 +111,7 @@ TEST(ContiguousIterator, Pointer) {
                 "contiguous iterator.");
 }
 
-TEST(ContiguousIterator, Vector) {
+TEST(ContiguousIterator, VectorInt) {
   static_assert(IsContiguousIterator<std::vector<int>::iterator>::value,
                 "Error: std::vector<int>::iterator should be considered a "
                 "contiguous iterator.");
@@ -131,6 +131,27 @@ TEST(ContiguousIterator, Vector) {
       "considered a contiguous iterator.");
 }
 
+TEST(ContiguousIterator, VectorString) {
+  static_assert(IsContiguousIterator<std::vector<std::string>::iterator>::value,
+                "Error: std::vector<std::string>::iterator should be "
+                "considered a contiguous iterator.");
+
+  static_assert(
+      IsContiguousIterator<std::vector<std::string>::const_iterator>::value,
+      "Error: std::vector<std::string>::const_iterator should be considered a "
+      "contiguous iterator.");
+
+  static_assert(
+      !IsContiguousIterator<std::vector<std::string>::reverse_iterator>::value,
+      "Error: std::vector<std::string>::reverse_iterator should not be "
+      "considered a contiguous iterator.");
+
+  static_assert(!IsContiguousIterator<
+                    std::vector<std::string>::const_reverse_iterator>::value,
+                "Error: std::vector<std::string>::const_reverse_iterator "
+                "should not be considered a contiguous iterator.");
+}
+
 TEST(ContiguousIterator, VectorBool) {
   static_assert(!IsContiguousIterator<std::vector<bool>::iterator>::value,
                 "Error: std::vector<bool>::iterator should not be considered "
@@ -151,7 +172,7 @@ TEST(ContiguousIterator, VectorBool) {
       "considered a contiguous iterator.");
 }
 
-TEST(ContiguousIterator, Array) {
+TEST(ContiguousIterator, ArrayInt) {
   static_assert(IsContiguousIterator<std::array<int, 1>::iterator>::value,
                 "Error: std::array<int, 1>::iterator should be considered a "
                 "contiguous iterator.");
@@ -171,6 +192,28 @@ TEST(ContiguousIterator, Array) {
       "considered a contiguous iterator.");
 }
 
+TEST(ContiguousIterator, ArrayString) {
+  static_assert(
+      IsContiguousIterator<std::array<std::string, 1>::iterator>::value,
+      "Error: std::array<std::string, 1>::iterator should be considered a "
+      "contiguous iterator.");
+
+  static_assert(
+      IsContiguousIterator<std::array<std::string, 1>::const_iterator>::value,
+      "Error: std::array<std::string, 1>::const_iterator should be considered "
+      "a contiguous iterator.");
+
+  static_assert(!IsContiguousIterator<
+                    std::array<std::string, 1>::reverse_iterator>::value,
+                "Error: std::array<std::string, 1>::reverse_iterator should "
+                "not be considered a contiguous iterator.");
+
+  static_assert(!IsContiguousIterator<
+                    std::array<std::string, 1>::const_reverse_iterator>::value,
+                "Error: std::array<std::string, 1>::const_reverse_iterator "
+                "should not be considered a contiguous iterator.");
+}
+
 TEST(ContiguousIterator, String) {
   static_assert(IsContiguousIterator<std::string::iterator>::value,
                 "Error: std::string:iterator should be considered a contiguous"
@@ -209,7 +252,7 @@ TEST(ContiguousIterator, String16) {
       "contiguous iterator.");
 }
 
-TEST(ContiguousIterator, Valarray) {
+TEST(ContiguousIterator, ValarrayInt) {
   static_assert(IsContiguousIterator<decltype(
                     std::begin(std::declval<std::valarray<int>&>()))>::value,
                 "Error: std::valarray<int>::iterator should be considered a "
@@ -222,6 +265,18 @@ TEST(ContiguousIterator, Valarray) {
       "contiguous iterator.");
 }
 
+TEST(ContiguousIterator, ValarrayString) {
+  static_assert(IsContiguousIterator<decltype(std::begin(
+                    std::declval<std::valarray<std::string>&>()))>::value,
+                "Error: std::valarray<std::string>::iterator should be "
+                "considered a contiguous iterator.");
+
+  static_assert(IsContiguousIterator<decltype(std::begin(
+                    std::declval<const std::valarray<std::string>&>()))>::value,
+                "Error: std::valarray<std::string>::const_iterator should be "
+                "considered a contiguous iterator.");
+}
+
 TEST(ContiguousIterator, StringPiece) {
   static_assert(
       IsContiguousIterator<base::StringPiece::const_iterator>::value,
@@ -234,7 +289,7 @@ TEST(ContiguousIterator, StringPiece) {
       "considered a contiguous iterator.");
 }
 
-TEST(ContiguousIterator, Span) {
+TEST(ContiguousIterator, SpanInt) {
   static_assert(IsContiguousIterator<base::span<int>::iterator>::value,
                 "Error: base::span<int>::iterator should be considered a "
                 "contiguous iterator.");
@@ -244,4 +299,15 @@ TEST(ContiguousIterator, Span) {
                 "considered a contiguous iterator.");
 }
 
+TEST(ContiguousIterator, SpanString) {
+  static_assert(IsContiguousIterator<base::span<std::string>::iterator>::value,
+                "Error: base::span<std::string>::iterator should be considered "
+                "a contiguous iterator.");
+
+  static_assert(
+      !IsContiguousIterator<base::span<std::string>::reverse_iterator>::value,
+      "Error: base::span<std::string>::reverse_iterator should not be "
+      "considered a contiguous iterator.");
+}
+
 }  // namespace base