[libcxx-commits] [PATCH] D116950: [libc++] Add the std::ranges::drop range adaptor object

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

I haven't really looked at the implementation yet, other than to say "ergh, this doesn't look like how I did it." ;)  I'll wait for test/forward-declaration updates before looking again, in case the test updates flush out any issues with the implementation.



================
Comment at: libcxx/include/CMakeLists.txt:204-205
   __functional/weak_result_type.h
+  __fwd/span.h
+  __fwd/string_view.h
   __hash_table
----------------
> This introduces forward declarations of string_view and span

On the cost side, there are several red flags associated with this part of the PR:
- requires unusual-looking modulemap changes
- introduces a new detail subdirectory (countermeasure: you could simply change the names to `__string/stringfwd.h` and `__span/spanfwd.h`, after moving the existing `__string` file out of the way; I expect that'd look fine)
- the idea of "forward declarations" in general (counterpoint: they already exist, you're just moving them a bit further away from their corresponding definitions). If we're talking about "future headaches," I would imagine "forward declaration, separated from its definition, accidentally bit-rots and drifts away from that definition" is a //vastly// more likely headache than anything involving circular dependencies between `span` and `drop_view`-etc.

How about renaming these to `__string/stringfwd.h` and `__span/spanfwd.h`, and perhaps granularizing the rest of `<string>` and/or `<span>` in a preliminary PR while you're at it? (I suggest `foofwd.h` by analogy with `iosfwd`. I can see a case for just `__foo/fwd.h`, but that might be ambiguous when `<foo>` contains multiple types and only some of them are being forward-declared in the detail header.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:52
+        std::same_as<std::span<int, std::dynamic_extent>> auto result = std::views::drop(span, 2);
+        assert(result.begin() == span.begin() + 2);
+      }
----------------
Here and throughout, can you think of a good way to test that `result.end()` also has its correct value?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:88
+        std::same_as<std::ranges::iota_view<int, int>> auto result = std::views::drop(view, 10);
+        assert(std::ranges::empty(result));
+      }
----------------
Simpler: `assert(result.empty())`, since `iota_view` provides an `.empty()` method?


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.drop/adaptor.pass.cpp:109-115
+  // Otherwise, views::drop(r, n) is ranges::drop_view{r, n}
+  {
+    int buff[] = {1, 2, 3, 4};
+    View view(buff, buff + 4);
+    std::same_as<std::ranges::drop_view<View>> auto result = std::views::drop(view, 2);
+    assert(result.begin() == buff + 2);
+  }
----------------
I'd like to see more value-category tests, i.e. `std::views::drop(static_cast<const View&&>(view), 2)` for all four interesting cvref-qualifications, and ditto for all four cvref-qualifications on at least one of the "passthrough" types.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116950