[libc++][test] Refactor SmallBasicString uses in range.lazy.split tests

[jloser]

Summary

The tests for std::ranges::lazy_split_view heavily use a wrapper class around
std::string because std::string was not constexpr until recently. Where
possible, remove the wrapper class and extra functionality no longer needed.
Remove libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h
and inline its one use remaining in
libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp.

[jloser]

Remove extra unused include

[philnik]

LGTM % nit, but I think @var-const should take a look here.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp

74–75

I think it makes more sense to replace the occurrences of StrRange with std::string. Ditto for string_view.

[jloser]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp

74–75

I'm OK with that. I was debating whether to even touch this at all in this patch (I could have added a base() to the end of begin() and end() functions like I did elsewhere in this patch). Do you have a preference, @var-const?

[jloser]

Fix CI for range.lazy.split/ctor.range.pass.cpp in C++20 mode. This is required since std::string_view constructor from a range only works in C++23 or later.

[jloser]

Rebase to see if the clang trunk (bootstrapping build) ICE goes away. Otherwise, I'll dig into it and report the Clang bug (likely)

[jloser]

Rebase to poke with newest Clang. I can repro the ICE on clang trunk, but it's not consistent. I think it's a miscompile by Clang in some cases.

[jloser]

rebase

[davidstone]

In D126663#3552370, @jloser wrote:

Rebase to poke with newest Clang. I can repro the ICE on clang trunk, but it's not consistent. I think it's a miscompile by Clang in some cases.

You get a consistent failure if you run against an assertions-enabled build of clang. https://github.com/llvm/llvm-project/issues/55867

[jloser]

In D126663#3557091, @davidstone wrote:

In D126663#3552370, @jloser wrote:

Rebase to poke with newest Clang. I can repro the ICE on clang trunk, but it's not consistent. I think it's a miscompile by Clang in some cases.

You get a consistent failure if you run against an assertions-enabled build of clang. https://github.com/llvm/llvm-project/issues/55867

Nice minimal - much smaller than when we looked at it earlier today! Thanks, @davidstone.

I'll try a bit to workaround it, otherwise, we'll just wait for the Clang patch as it's a recent regression (works in Clang 14, for example).

[jloser]

Workaround the ICE in clang trunk in InputView

[jloser]

Workaround clang trunk ICE for ForwardDiffView as well

[jloser]

ping @var-const, CI is passing!

[var-const]

@jloser Sorry about the slow response! I'll take a look tomorrow

[var-const]

Generally LGTM (and thanks a lot for doing this cleanup!), but I have a couple of questions -- perhaps we can clean up even more stuff, but if not, this patch looks pretty good already.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp

74–75

The current state looks good to me.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp

34

What prevents replacing this class with std::string as well?

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp

34–36

Why is calling equal necessary instead of ==?

[jloser]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp

34

I think it's the different types of things we're working with the Char and Str types in is_equal function template. Specifically, the family of types it takes on is "stringlike" but we take advantage of that extra layer of conversion when constructing the BasicSmallString (note the three different constructors all of which we use - specifically when going from a string_view-like type to a string for example).

I played with it for a while and didn't find a way to remove the dependency. I do recommend you or @philnik play with it as a follow up for a bit to see if you come up with something clever as you're both more knowledgable in the ranges area. I really would like to get rid of is_equal entirely and just use std::ranges::equal, but the default projection won't work for similar reasons as to why we have BasicSmallString to begin with (and the view type's iterator/sentinel won't satisfy input_range needed for std::ranges::equal).

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp

34–36

There's no valid operator== now. The error is:

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp:35:15: error: invalid operands to binary expression ('std::ranges::lazy_split_view<ForwardDiffView, ForwardDiffView>::__outer_iterator<false>::value_type' and 'basic_string<char>')
    assert(*i == "abc"s);

In the previous code, it worked because of

constexpr bool operator==(const BasicSmallString& lhs, std::string_view rhs)

in small_string.h if I understand correctly. Now that we removed SmallString, we don't have that comparison operator. Similar story in the other call sites that were made to use std::ranges::equal.

Does that make sense?

[jloser]

ping @philnik since @var-const has taken a look at this patch now.

[philnik]

Although the ranges::equal aren't perfect I think this is an improvement overall, so LGTM. We can further improve the tests in other patches if we want.

[var-const]

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp

34–36

Ah, right, basically dereferencing returns some helper type instead of a string, and we don't have the constructor taking a range for std::string yet. Thanks for explaining.

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.copy_move.pass.cpp

 
 // Test the implicitly-generated copy and move constructors since `lazy_split_view` has non-trivial members.
 
 #include <ranges>
 
 #include <cassert>
 #include <string_view>
 #include <utility>
-#include "small_string.h"
 #include "types.h"
 
-constexpr bool operator==(const InputView& lhs, const InputView& rhs) {
-  return SmallString(lhs) == SmallString(rhs);
-}
-
 constexpr bool test() {
   // Can copy `lazy_split_view`.
   {
     // Forward range.
     {
       std::ranges::lazy_split_view<std::string_view, std::string_view> v1("abc def", " ");
       auto v2 = v1;
       assert(v2.base() == v1.base());

libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp

 // template <input_range Range>
 //   requires constructible_from<View, views::all_t<Range>> &&
 //             constructible_from<Pattern, single_view<range_value_t<Range>>>
 // constexpr lazy_split_view(Range&& r, range_value_t<Range> e);
 
 #include <ranges>
 
 #include <cassert>
+#include <string>
 #include <string_view>
 #include <type_traits>
 #include <utility>
-#include "small_string.h"
 #include "types.h"
 
 struct ElementWithCounting {
   int* times_copied = nullptr;
   int* times_moved = nullptr;
 
   constexpr ElementWithCounting(int& copies_ctr, int& moves_ctr) : times_copied(&copies_ctr), times_moved(&moves_ctr) {}
 
struct RangeWithCounting {
   constexpr const ElementWithCounting* end() const { return nullptr; }
 
   constexpr RangeWithCounting& operator=(const RangeWithCounting&) = default;
   constexpr RangeWithCounting& operator=(RangeWithCounting&&) = default;
   constexpr bool operator==(const RangeWithCounting&) const { return true; }
 };
 static_assert( std::ranges::forward_range<RangeWithCounting>);
 static_assert(!std::ranges::view<RangeWithCounting>);
 
-struct StrRange {
-  SmallString buffer_;
-  constexpr explicit StrRange() = default;
-  constexpr StrRange(const char* ptr) : buffer_(ptr) {}
-  constexpr const char* begin() const { return buffer_.begin(); }
-  constexpr const char* end() const { return buffer_.end(); }
-  constexpr bool operator==(const StrRange& rhs) const { return buffer_ == rhs.buffer_; }
-};
-static_assert( std::ranges::random_access_range<StrRange>);
-static_assert(!std::ranges::view<StrRange>);
-static_assert( std::is_copy_constructible_v<StrRange>);
-
 struct StrView : std::ranges::view_base {
-  SmallString buffer_;
+  std::string_view buffer_;
   constexpr explicit StrView() = default;
   constexpr StrView(const char* ptr) : buffer_(ptr) {}
+  // Intentionally don't forward to range constructor for std::string_view since
+  // this test needs to work on C++20 as well and the range constructor is only for
+  // C++23 and later.
   template <std::ranges::range R>
-  constexpr StrView(R&& r) : buffer_(std::forward<R>(r)) {}
+  constexpr StrView(R&& r) : buffer_(r.begin(), r.end()) {}
   constexpr const char* begin() const { return buffer_.begin(); }
   constexpr const char* end() const { return buffer_.end(); }
   constexpr bool operator==(const StrView& rhs) const { return buffer_ == rhs.buffer_; }
 };
 static_assert( std::ranges::random_access_range<StrView>);
 static_assert( std::ranges::view<StrView>);
 static_assert( std::is_copy_constructible_v<StrView>);
 
 constexpr bool test() {
   {
     using V = std::ranges::lazy_split_view<StrView, StrView>;
 
-    // Calling the constructor with `(StrRange, range_value_t)`.
+    // Calling the constructor with `(std::string, range_value_t)`.
     {
-      StrRange input;
+      std::string input;
       V v(input, ' ');
       assert(v.base() == input);
     }
 
     // Calling the constructor with `(StrView, range_value_t)`.
     {
       StrView input("abc def");
       V v(input, ' ');

libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp

 #include <algorithm>
 #include <array>
 #include <cassert>
 #include <map>
 #include <string>
 #include <string_view>
 #include <utility>
 #include <vector>
-#include "small_string.h"
 #include "types.h"
 
+// A constexpr-friendly lightweight string, primarily useful for comparisons.
+// Unlike `std::string_view`, it copies the given string into an
+// internal buffer and can work with non-contiguous inputs.
+template <class Char>
+class BasicSmallString {
+  std::basic_string<Char> buffer_{};
+
+public:
+  constexpr BasicSmallString(std::basic_string_view<Char> v) : buffer_(v) {}
+
+  template <class I, class S>
+  constexpr BasicSmallString(I b, const S& e) {
+    for (; b != e; ++b) {
+      buffer_ += *b;
+    }
+  }
+
+  template <std::ranges::range R>
+  constexpr BasicSmallString(R&& from) : BasicSmallString(from.begin(), from.end()) {}
+
+  friend constexpr bool operator==(const BasicSmallString& lhs, const BasicSmallString& rhs) {
+    return lhs.buffer_ == rhs.buffer_;
+  }
+};
+
 template <std::ranges::view View, std::ranges::range Expected>
 constexpr bool is_equal(View& view, const Expected& expected) {
   using Char = std::ranges::range_value_t<std::ranges::range_value_t<View>>;
   using Str = BasicSmallString<Char>;
 
   auto actual_it = view.begin();
   auto expected_it = expected.begin();
   for (; actual_it != view.end() && expected_it != expected.end(); ++actual_it, ++expected_it) {

 }
 
 template <class T, class Separator, class U, size_t M>
 constexpr bool test_with_piping(T&& input, Separator&& separator, std::array<U, M> expected) {
   auto expected_it = expected.begin();
   for (auto e : input | std::ranges::views::lazy_split(separator)) {
     if (expected_it == expected.end())
       return false;
-    if (SmallString(e) != *expected_it)
+    if (!std::ranges::equal(e, *expected_it))
       return false;
 
     ++expected_it;
   }
 
   return expected_it == expected.end();
 }
 

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03, c++11, c++14, c++17
 // UNSUPPORTED: libcpp-has-no-incomplete-ranges
 
 // constexpr outer-iterator::value-type outer-iterator::operator*() const;
 
 #include <ranges>
 
+#include <algorithm>
 #include <cassert>
+#include <string>
 #include <string_view>
 #include <type_traits>
-#include "../small_string.h"
 #include "../types.h"
 
 template <class View, class Separator>
 constexpr void test_one(Separator sep) {
+  using namespace std::string_literals;
   using namespace std::string_view_literals;
 
   View v("abc def ghi"sv, sep);
 
   // Non-const iterator.
   {
     auto i = v.begin();
     static_assert(!std::is_reference_v<decltype(*i)>);
-    assert(SmallString(*i) == "abc"_str);
-    assert(SmallString(*(++i)) == "def"_str);
-    assert(SmallString(*(++i)) == "ghi"_str);
+    assert(std::ranges::equal(*i, "abc"s));
+    assert(std::ranges::equal(*(++i), "def"s));
+    assert(std::ranges::equal(*(++i), "ghi"s));
   }
 
   // Const iterator.
   {
     const auto ci = v.begin();
     static_assert(!std::is_reference_v<decltype(*ci)>);
-    assert(SmallString(*ci) == "abc"_str);
+    assert(std::ranges::equal(*ci, "abc"s));
   }
 }
 
 constexpr bool test() {
   // `View` is a forward range.
   test_one<SplitViewDiff>(" ");
   test_one<SplitViewInput>(' ');
 

libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/increment.pass.cpp

 // constexpr outer-iterator& outer-iterator::operator++();
 // constexpr decltype(auto) outer-iterator::operator++(int);
 
 // Note that corner cases are tested in `range.lazy.split/general.pass.cpp`.
 
 #include <ranges>
 
 #include <algorithm>
-#include <string_view>
-#include "../small_string.h"
+#include <cassert>
+#include <string>
 #include "../types.h"
 
 constexpr bool test() {
+  using namespace std::string_literals;
   // Can call `outer-iterator::operator++`; `View` is a forward range.
   {
     SplitViewForward v("abc def ghi", " ");
 
     // ++i
     {
       auto i = v.begin();
-      assert(*i == "abc"_str);
+      assert(std::ranges::equal(*i, "abc"s));
 
       decltype(auto) i2 = ++i;
       static_assert(std::is_lvalue_reference_v<decltype(i2)>);
       assert(&i2 == &i);
-      assert(*i2 == "def"_str);
+      assert(std::ranges::equal(*i2, "def"s));
     }
 
     // i++
     {
       auto i = v.begin();
-      assert(*i == "abc"_str);
+      assert(std::ranges::equal(*i, "abc"s));
 
       decltype(auto) i2 = i++;
       static_assert(!std::is_reference_v<decltype(i2)>);
-      assert(*i2 == "abc"_str);
-      assert(*i == "def"_str);
+      assert(std::ranges::equal(*i2, "abc"s));
+      assert(std::ranges::equal(*i, "def"s));
     }
   }
 
   // Can call `outer-iterator::operator++`; `View` is an input range.
   {
     SplitViewInput v("abc def ghi", ' ');
 
     // ++i
     {
       auto i = v.begin();
-      assert(*i == "abc"_str);
+      assert(std::ranges::equal(*i, "abc"s));
 
       decltype(auto) i2 = ++i;
       static_assert(std::is_lvalue_reference_v<decltype(i2)>);
       assert(&i2 == &i);
-      assert(*i2 == "def"_str);
+      assert(std::ranges::equal(*i2, "def"s));
     }
 
     // i++
     {
       auto i = v.begin();
-      assert(*i == "abc"_str);
+      assert(std::ranges::equal(*i, "abc"s));
 
       static_assert(std::is_void_v<decltype(i++)>);
       i++;
-      assert(*i == "def"_str);
+      assert(std::ranges::equal(*i, "def"s));
     }
   }
 
   return true;
 }
 
 int main(int, char**) {
   test();
   static_assert(test());
 
   return 0;
 }

libcxx/test/std/ranges/range.adaptors/range.lazy.split/small_string.h

-//===----------------------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_LAZY_SPLIT_SMALL_STRING_H
-#define TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_LAZY_SPLIT_SMALL_STRING_H
-
-#include <algorithm>
-#include <cassert>
-#include <ranges>
-#include <string_view>
-
-// A constexpr-friendly lightweight string, primarily useful for comparisons.
-// Unlike `std::string`, all functions are `constexpr`. Unlike `std::string_view`, it copies the given string into an
-// internal buffer and can work with non-contiguous inputs.
-//
-// TODO(var-const): remove once https://reviews.llvm.org/D110598 lands and `std::string` can be used instead of this
-// class.
-template <class Char>
-class BasicSmallString {
-  constexpr static int N = 32;
-  Char buffer_[N] = {};
-  size_t size_ = 0;
-
-public:
-  // Main constructors.
-
-  constexpr BasicSmallString() = default;
-
-  constexpr BasicSmallString(std::basic_string_view<Char> v) : size_(v.size()) {
-    assert(size_ < N);
-    if (size_ == 0) return;
-
-    std::copy(v.begin(), v.end(), buffer_);
-  }
-
-  template <class I, class S>
-  constexpr BasicSmallString(I b, const S& e) {
-    for (; b != e; ++b) {
-      buffer_[size_++] = *b;
-      assert(size_ < N);
-    }
-  }
-
-  // Delegating constructors.
-
-  constexpr BasicSmallString(const Char* ptr, size_t size) : BasicSmallString(std::basic_string_view<Char>(ptr, size)) {
-  }
-
-  template <std::ranges::range R>
-  constexpr BasicSmallString(R&& from) : BasicSmallString(from.begin(), from.end()) {
-  }
-
-  // Iterators.
-
-  constexpr Char* begin() { return buffer_; }
-  constexpr Char* end() { return buffer_ + size_; }
-  constexpr const Char* begin() const { return buffer_; }
-  constexpr const Char* end() const { return buffer_ + size_; }
-
-  friend constexpr bool operator==(const BasicSmallString& lhs, const BasicSmallString& rhs) {
-    return lhs.size_ == rhs.size_ && std::equal(lhs.buffer_, lhs.buffer_ + lhs.size_, rhs.buffer_);
-  }
-  friend constexpr bool operator==(const BasicSmallString& lhs, std::string_view rhs) {
-    return lhs == BasicSmallString(rhs);
-  }
-};
-
-using SmallString = BasicSmallString<char>;
-
-inline constexpr SmallString operator "" _str(const char* ptr, size_t size) {
-  return SmallString(ptr, size);
-}
-
-#endif // TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_LAZY_SPLIT_SMALL_STRING_H

libcxx/test/std/ranges/range.adaptors/range.lazy.split/types.h

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_LAZY_SPLIT_TYPES_H
 #define TEST_STD_RANGES_RANGE_ADAPTORS_RANGE_LAZY_SPLIT_TYPES_H
 
 #include <concepts>
 #include <ranges>
+#include <string>
 #include <string_view>
 #include <type_traits>
-#include "small_string.h"
 #include "test_macros.h"
 #include "test_iterators.h"
 
 // CopyableView
 
 struct CopyableView : std::ranges::view_base {
   std::string_view view_;
   constexpr explicit CopyableView() = default;

 static_assert( std::ranges::view<ForwardView>);
 static_assert(!std::is_copy_constructible_v<ForwardView>);
 static_assert( std::is_move_constructible_v<ForwardView>);
 
 // ForwardDiffView
 
 // Iterator types differ based on constness of this class.
 struct ForwardDiffView : std::ranges::view_base {
-  SmallString buffer_;
+  std::string buffer_;
   constexpr explicit ForwardDiffView() = default;
   constexpr ForwardDiffView(const char* ptr) : ForwardDiffView(std::string_view(ptr)) {}
-  constexpr ForwardDiffView(std::string_view v) : buffer_(v) {}
+  constexpr ForwardDiffView(std::string_view v) {
+    // Workaround https://github.com/llvm/llvm-project/issues/55867
+    buffer_ = v;
+  }
   constexpr ForwardDiffView(ForwardDiffView&&) = default;
   constexpr ForwardDiffView& operator=(ForwardDiffView&&) = default;
   constexpr ForwardDiffView(const ForwardDiffView&) = default;
   constexpr ForwardDiffView& operator=(const ForwardDiffView&) = default;
-  constexpr forward_iterator<char*> begin() { return forward_iterator<char*>(buffer_.begin()); }
-  constexpr forward_iterator<char*> end()  { return forward_iterator<char*>(buffer_.end()); }
-  constexpr forward_iterator<const char*> begin() const { return forward_iterator<const char*>(buffer_.begin()); }
-  constexpr forward_iterator<const char*> end() const { return forward_iterator<const char*>(buffer_.end()); }
+  constexpr forward_iterator<char*> begin() { return forward_iterator<char*>(buffer_.begin().base()); }
+  constexpr forward_iterator<char*> end()  { return forward_iterator<char*>(buffer_.end().base()); }
+  constexpr forward_iterator<const char*> begin() const { return forward_iterator<const char*>(buffer_.begin().base()); }
+  constexpr forward_iterator<const char*> end() const { return forward_iterator<const char*>(buffer_.end().base()); }
 };
 static_assert( std::ranges::forward_range<ForwardView>);
 static_assert( std::ranges::forward_range<const ForwardView>);
 static_assert( std::ranges::view<ForwardView>);
 static_assert(!std::same_as<std::ranges::iterator_t<ForwardDiffView>, std::ranges::iterator_t<const ForwardDiffView>>);
 
 // ForwardOnlyIfNonConstView
 

 };
 static_assert( std::ranges::forward_range<ForwardOnlyIfNonConstView>);
 static_assert(!std::ranges::forward_range<const ForwardOnlyIfNonConstView>);
 static_assert( std::ranges::view<ForwardOnlyIfNonConstView>);
 
 // InputView
 
 struct InputView : std::ranges::view_base {
-  SmallString buffer_;
+  std::string buffer_;
 
   constexpr InputView() = default;
   constexpr InputView(const char* s) : InputView(std::string_view(s)) {}
-  constexpr InputView(std::string_view v) : buffer_(v) {}
+  constexpr InputView(std::string_view v) {
+    // Workaround https://github.com/llvm/llvm-project/issues/55867
+    buffer_ = v;
+  }
 
-  constexpr cpp20_input_iterator<char*> begin() { return cpp20_input_iterator<char*>(buffer_.begin()); }
+  constexpr cpp20_input_iterator<char*> begin() { return cpp20_input_iterator<char*>(buffer_.begin().base()); }
   constexpr sentinel_wrapper<cpp20_input_iterator<char*>> end() {
-    return sentinel_wrapper(cpp20_input_iterator<char*>(buffer_.end()));
+    return sentinel_wrapper(cpp20_input_iterator<char*>(buffer_.end().base()));
   }
   constexpr cpp20_input_iterator<const char*> begin() const {
-    return cpp20_input_iterator<const char*>(buffer_.begin());
+    return cpp20_input_iterator<const char*>(buffer_.begin().base());
   }
   constexpr sentinel_wrapper<cpp20_input_iterator<const char*>> end() const {
-    return sentinel_wrapper(cpp20_input_iterator<const char*>(buffer_.end()));
+    return sentinel_wrapper(cpp20_input_iterator<const char*>(buffer_.end().base()));
+  }
+  friend constexpr bool operator==(const InputView& lhs, const InputView& rhs) {
+    return lhs.buffer_ == rhs.buffer_;
   }
 };
 
 static_assert(std::ranges::input_range<InputView>);
 static_assert(std::ranges::input_range<const InputView>);
 static_assert(std::ranges::view<InputView>);
 
 // ForwardTinyView