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

[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.

Please also `git grep` the tests for various terms such as `directory_iterator`, to see if there's any TODOs that can be enabled/uncommented at this point. I didn't see any in a very quick skim, but I imagine there must be some.



================
Comment at: libcxx/docs/Status/RangesIssues.csv:88
 `LWG3470 <https://wg21.link/LWG3470>`__,"``convertible-to-non-slicing`` seems to reject valid case",,
-`LWG3480 <https://wg21.link/LWG3480>`__,"``directory_iterator`` and ``recursive_directory_iterator`` are not C++20 ranges",,
+`LWG3480 <https://wg21.link/LWG3480>`__,"``directory_iterator`` and ``recursive_directory_iterator`` are not C++20 ranges",|Complete|,14.0
 `LWG3535 <https://wg21.link/LWG3535>`__,"``join_view::iterator::iterator_category`` and ``::iterator_concept`` lie",,
----------------
`"|Complete|","14.0"` for consistency with other lines (e.g. line 73)


================
Comment at: libcxx/include/filesystem:51
     directory_iterator begin(directory_iterator iter) noexcept;
-    directory_iterator end(const directory_iterator&) noexcept;
+    directory_iterator end(directory_iterator&) noexcept;
 
----------------
Remove the `&` too. Ditto line 57.
The gist is that we're going to pass by value instead of by const-reference.


================
Comment at: libcxx/include/filesystem:237
+    inline constexpr bool ranges::enable_view<filesystem::recursive_directory_iterator> = true;
+}  // namespaces std
 
----------------
(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;

*/
```


================
Comment at: libcxx/include/filesystem:2883
 inline _LIBCPP_INLINE_VISIBILITY directory_iterator
-end(const directory_iterator&) noexcept {
+end(directory_iterator&) noexcept {
   return directory_iterator();
----------------
Add a test that catches this bug.
Then, remove `&`.


================
Comment at: libcxx/include/filesystem:3015
 inline _LIBCPP_INLINE_VISIBILITY recursive_directory_iterator
-end(const recursive_directory_iterator&) noexcept {
+end(recursive_directory_iterator&) noexcept {
   return recursive_directory_iterator();
----------------
Ditto.


================
Comment at: libcxx/include/filesystem:3027-3037
+#if !defined(_LIBCPP_HAS_NO_RANGES)
+template<>
+inline constexpr bool _VSTD::ranges::enable_borrowed_range<_VSTD_FS::directory_iterator> = true;
+template <>
+inline constexpr bool _VSTD::ranges::enable_borrowed_range<_VSTD_FS::recursive_directory_iterator> = true;
+
+template <>
----------------
Please move these additions up to right after `_LIBCPP_END_NAMESPACE_FILESYSTEM`, right before `#endif // !_LIBCPP_CXX03_LANG`.


================
Comment at: libcxx/test/std/input.output/filesystems/class.directory_iterator/directory_iterator.nonmembers/begin_end.pass.cpp:39
     ASSERT_SAME_TYPE(decltype(end(d)), directory_iterator);
-    ASSERT_NOEXCEPT(end(std::move(d)));
+    ASSERT_NOEXCEPT(end(d));
 }
----------------
This test looks like it started out bogus. Let's make it
```
    ASSERT_SAME_TYPE(decltype(begin(d)), directory_iterator);
    ASSERT_SAME_TYPE(decltype(begin(std::move(d))), directory_iterator);
    ASSERT_NOEXCEPT(begin(d));
    ASSERT_NOEXCEPT(begin(std::move(d)));
```
and likewise for `end`.


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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111644