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

[Joe Loser via Phabricator via libcxx-commits]

jloser marked an inline comment as done.
jloser 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;
----------------
jloser wrote:
> Quuxplusone wrote:
> > 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.
> Fixed the spacing in `template <>` here, in the synopsis, and the tests.
> 
> As for the `_LIBCPP_TEMPLATE_VIS`, I don't know. I added it to the class, but can we get away with adding it just to these specializations? @ldionne 
FWIW `span` doesn't have `_LIBCPP_TEMPLATE_VIS` on the similar variable template specializations for `span`, so I removed it for now unless @ldionne tells me I need it.


================
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
+
----------------
jloser wrote:
> Quuxplusone wrote:
> > Can we do `UNSUPPORTED: libcpp-no-concepts` instead? Ditto throughout.
> Let's see what BuildKite has to say -- just tried.
Looks like the answer is "no" for these `range_concept_conformance.compile.pass.cpp`. AppleClang 12.0.0 supports concepts but not ranges and this test requires both. Ditto for the `recursive_directory_iterator` one. For the other test files, I removed the extra `UNSUPPORTED` clause.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111644