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

[Joe Loser via Phabricator via libcxx-commits]

jloser marked 7 inline comments as done.
jloser added inline comments.


================
Comment at: libcxx/include/filesystem:237
+    inline constexpr bool ranges::enable_view<filesystem::recursive_directory_iterator> = true;
+}  // namespaces std
 
----------------
Quuxplusone wrote:
> (1) `namespaces` should be `namespace` (I think this was originally intentional, but strikes me as too cute). Let's change line 14 to use `namespace std::filesystem` for simplicity.
> (2) I think it would read better to just leave the specializations outside:
> ```
> namespace std::filesystem {
>     ~~~
> } // namespace std::filesystem
> 
> template<>
> inline constexpr bool std::ranges::enable_borrowed_range<std::filesystem::directory_iterator> = true;
> template<>
> inline constexpr bool std::ranges::enable_borrowed_range<std::filesystem::recursive_directory_iterator> = true;
> 
> template<>
> inline constexpr bool std::ranges::enable_view<std::filesystem::directory_iterator> = true;
> template<>
> inline constexpr bool std::ranges::enable_view<std::filesystem::recursive_directory_iterator> = true;
> 
> */
> ```
Fixed. The "namespaces" was a bit cute -- didn't even notice :)


================
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;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111644