[libcxx-commits] [PATCH] D126663: [libc++][test] Refactor SmallBasicString uses in range.lazy.split tests

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const accepted this revision as: var-const.
var-const added a comment.

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.



================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/ctor.range.pass.cpp:75
 
-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_; }
-};
+using StrRange = std::string;
 static_assert( std::ranges::random_access_range<StrRange>);
----------------
jloser wrote:
> philnik wrote:
> > I think it makes more sense to replace the occurrences of `StrRange` with `std::string`. Ditto for `string_view`.
> 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?
The current state looks good to me.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/general.pass.cpp:34
+template <class Char>
+class BasicSmallString {
+  std::basic_string<Char> buffer_{};
----------------
What prevents replacing this class with `std::string` as well?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.lazy.split/range.lazy.split.outer/deref.pass.cpp:34
     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));
----------------
Why is calling `equal` necessary instead of `==`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126663/new/

https://reviews.llvm.org/D126663