[libcxx-commits] [PATCH] D111644: [libc++] LWG3480: make (recursive)_directory_iterator C++20 ranges

Wed Oct 13 09:01:59 PDT 2021

Quuxplusone added inline comments.

================
Comment at: libcxx/include/filesystem:3024
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+template<>
+inline constexpr bool _VSTD::ranges::enable_borrowed_range<_VSTD_FS::directory_iterator> = true;
----------------
Nitpick: `template <>` for consistency. (FWIW, I personally write `template<>` consistently in my own code. But libc++ more often uses `template <>`; and more importantly, lines 3026, 3029, 3031 already use `template <>`.)

Also, do you need `_LIBCPP_TEMPLATE_VIS` throughout? I think so, but I'm not sure.

================
Comment at: libcxx/test/std/input.output/filesystems/class.directory_iterator/range_concept_conformance.compile.pass.cpp:12-13

-// directory_iterator, recursive_directory_iterator
+// AppleClang 12.0.0 doesn't fully support ranges/concepts
+// XFAIL: apple-clang-12.0.0
+
----------------
Can we do `UNSUPPORTED: libcpp-no-concepts` instead? Ditto throughout.

================
Comment at: libcxx/test/std/input.output/filesystems/class.directory_iterator/range_concept_conformance.compile.pass.cpp:30-36
 static_assert(std::same_as<std::ranges::iterator_t<fs::directory_iterator const>, fs::directory_iterator>);
 static_assert(std::ranges::common_range<fs::directory_iterator const>);
 static_assert(std::ranges::input_range<fs::directory_iterator const>);
 static_assert(!std::ranges::view<fs::directory_iterator const>);
 static_assert(!std::ranges::sized_range<fs::directory_iterator const>);
-static_assert(!std::ranges::borrowed_range<fs::directory_iterator const>);
-static_assert(!std::ranges::viewable_range<fs::directory_iterator const>);
+static_assert(std::ranges::borrowed_range<fs::directory_iterator const>);
+static_assert(std::ranges::viewable_range<fs::directory_iterator const>);
----------------
Pre-existing: It would make much more sense for us to check all these concepts on `fs::directory_iterator&` and `const fs::directory_iterator&`, rather than on `const fs::directory_iterator`. It's very common for Ranges code to depend on `SomeConcept<X&>`, and very very uncommon for it to care about `SomeConcept<const X>`:
```
void foo(std::ranges::range auto&& r);
int main() {
    Widget w;
    const Widget cw;
    foo(w);  // requires range<Widget&>
    foo(Widget());  // requires range<Widget>
    foo(cw);  // requires range<const Widget&>
    foo(std::move(cw));  // requires range<const Widget> ...but who writes this?
}
```
Probably super annoying to do consistently, so this is not a blocker, just a complaint.

================
Comment at: libcxx/test/std/input.output/filesystems/fs.filesystem.synopsis/enable_borrowed_range.compile.pass.cpp:22
+//template<>
+//inline constexpr bool ranges::enable_view<filesystem::recursive_directory_iterator> = true;
+
----------------
jloser wrote:
> Quuxplusone wrote:
> > I suggest splitting this file up into `borrowed_range.compile.pass.cpp` and `view.compile.pass.cpp`.
> > Furthermore, I think we need //additional// tests verifying that:
> > - `ranges::borrowed_range<foo_directory_iterator>` is satisfied
> > - `ranges::view<foo_directory_iterator>` is satisfied
> > - `ranges::begin(d)` and `ranges::end(d)` have the correct behavior (and compile successfully) — but technically this is implied by `ranges::borrowed_range` (which implies `ranges::range`), so if you can't find a logical place to test these, then I don't really mind omitting these.
> > 
> Do you mind elaborating on the first two bullet points, please? As far as the third, I started working in `libcxx/test/std/input.output/filesystems/class.directory_iterator/directory_iterator.nonmembers/ranges_begin_end.pass.cpp` but didn't push it all the way through. I'm not sure I see the value in testing `ranges::begin(d)` and `ranges::end(d)`. If we have existing tests verifying the correct behavior for nonmember begin/end, then it seems a bit much to also test `ranges::begin(d)` and `range::end(d)` IMO. What do you think? Note the "compiles successfully" should be covered by the range concept conformance tests I came across and made pass AFAICT.
[Long comment already written, but I think it's moot now that you've discovered `class.directory_iterator/range_concept_conformance.compile.pass.cpp`]

All three bullet points are kind of the same flavor; it's about the continuum from "testing the very high levels (integration tests) (why?)" down to "testing the very low levels (unit tests) (how?)." Your current PR does the "unit test" — verify that `enable_borrowed_range<directory_iterator>` is true — which is concerned with the very low level mechanism by which the higher-level goal is supposedly achieved. I'm saying, it would be useful to also test the higher-level goal itself — verify that `borrowed_range<directory_iterator>` is true. This guards against the possibility that we messed up some //other// minor detail (maybe one we didn't even know about) and thus failed to achieve the higher-level goal.

Similarly with `begin` and `ranges::begin`: the goal of providing ADL `begin` is to enable `ranges::begin`, but if we messed up some other minor detail, then maybe `ranges::begin` still wouldn't work, so it'd be nice to test it.  (However, as I said above, I'm OK with omitting the `begin`/`end` tests. I'm pretty comfortable with the assumption that anything-with-an-ADL-`begin` will also support `ranges::begin`, by definition. A lot of this comes down to how comfortable each of us is personally with the various mazey pathways through the Ranges library. I find it reasonably obvious that if `begin` works then `ranges::begin` will work; //you// may find it reasonably obvious that if `enable_borrowed_range` works then `borrowed_range` itself will work.)

It's a continuum, and there are definitely some tests in the test suite right now where I personally would //not// have gone so high-level-integration-testy on the continuum. For example, `std/containers/sequences/deque/iterator_concept_conformance.compile.pass.cpp` strikes me as overkill. However, in this case I personally //would// go even a little //higher// than you've gone — and in fact, `std/input.output/filesystems/class.directory_iterator/range_concept_conformance.compile.pass.cpp` is the right place to put these tests...

...Oh look, these tests already exist, and someone had `XFAIL: *`'ed them. How fun. 😛 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111644