Implement LWG3480 which enables directory_iterator and
recursive_directory_iterator to be both a borrowed_range and a
view.
Details
Details
- Reviewers
ldionne • Quuxplusone Mordante - Group Reviewers
Restricted Project - Commits
- rG1fa27f2a10e8: [libc++] LWG3480: make (recursive_)directory_iterator C++20 ranges
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
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.
| libcxx/docs/Status/RangesIssues.csv | ||
|---|---|---|
| 88 | "|Complete|","14.0" for consistency with other lines (e.g. line 73) | |
| libcxx/include/filesystem | ||
| 51 | Remove the & too. Ditto line 57. | |
| 233 | (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. 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;
*/ | |
| 2883 | Add a test that catches this bug. | |
| 3015 | Ditto. | |
| 3039–3049 | Please move these additions up to right after _LIBCPP_END_NAMESPACE_FILESYSTEM, right before #endif // !_LIBCPP_CXX03_LANG. | |
| libcxx/test/std/input.output/filesystems/class.directory_iterator/directory_iterator.nonmembers/begin_end.pass.cpp | ||
| 43 | 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. | |
| libcxx/test/std/input.output/filesystems/fs.filesystem.synopsis/enable_borrowed_range.compile.pass.cpp | ||
| 23 | I suggest splitting this file up into borrowed_range.compile.pass.cpp and view.compile.pass.cpp.
| |
Comment Actions
- Address Arthur's feedback
- Don't XFAIL: * the range concept conformance tests for directory_iterator and recursive_directory_iterator. Split them up and adjust the static asserts so the tests pass.
- Extend testing for enable_view and enable_borrowed range to test ref and const qualified variants
| libcxx/include/filesystem | ||
|---|---|---|
| 233 | Fixed. The "namespaces" was a bit cute -- didn't even notice :) | |
| libcxx/test/std/input.output/filesystems/fs.filesystem.synopsis/enable_borrowed_range.compile.pass.cpp | ||
| 23 | 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. | |
| libcxx/include/filesystem | ||
|---|---|---|
| 3024 | 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. | |
| libcxx/test/std/input.output/filesystems/class.directory_iterator/range_concept_conformance.compile.pass.cpp | ||
| 12–13 | Can we do UNSUPPORTED: libcpp-no-concepts instead? Ditto throughout. | |
| 26–34 | 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. | |
| libcxx/test/std/input.output/filesystems/fs.filesystem.synopsis/enable_borrowed_range.compile.pass.cpp | ||
| 23 | [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. 😛 | |
Comment Actions
Test value type, reference type, and const reference types in range concept conformance tests
Fix whitespacing with template<> in synopsis and in tests
| libcxx/include/filesystem | ||
|---|---|---|
| 3024 | 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 | |
| libcxx/test/std/input.output/filesystems/class.directory_iterator/range_concept_conformance.compile.pass.cpp | ||
| 12–13 | Let's see what BuildKite has to say -- just tried. | |
Comment Actions
Remove _LIBCPP_TEMPLATE_VIS
Change range_concept_conformance.compile.pass to be UNSUPPORTED: libcpp-has-no-incomplete-ranges
| libcxx/include/filesystem | ||
|---|---|---|
| 3024 | 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. | |
| libcxx/test/std/input.output/filesystems/class.directory_iterator/range_concept_conformance.compile.pass.cpp | ||
| 12–13 | 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. | |
Comment Actions
LGTM as-is with nitpicks comments about formatting. I suggest we tackle visibility as a global commit.
| libcxx/include/filesystem | ||
|---|---|---|
| 3024 | _LIBCPP_TEMPLATE_VIS makes the vague linkage objects of a type visible. Here we're talking about a variable so you definitely don't need it. In fact, we have a pre-existing issue that non of our variable templates are visibility-annotated, which leads to a bunch of weak symbols leaking from user programs when they use those variables. That was reported to me internally by our dynamic linker folks, who are intimately aware of these sorts of issues because the dynamic linker is the one having to do the job of de-duplicating the symbols. Well, I guess in theory the more correct behavior is to give default visibility to those variable templates such that they have the same address if used in two different dylibs, however in practice I believe it's better to reduce the number of weak symbols since anyone relying on the address of these variables templates for their correctness is doing something really wrong. To illustrate the situation, consider this: cat <<EOF | clang++ -xc++ - -std=c++20 -fvisibility=hidden -shared -o liba.dylib && nm -omg liba.dylib | c++filt
#include <type_traits>
__attribute__((visibility("default")))
void const* a() { return &std::is_integral_v<int>; }
EOF
cat <<EOF | clang++ -xc++ - -std=c++20 -fvisibility=hidden -shared -o libb.dylib && nm -omg libb.dylib | c++filt
#include <type_traits>
__attribute__((visibility("default")))
void const* b() { return &std::is_integral_v<int>; }
EOF
cat <<EOF | clang++ -xc++ - -std=c++20 -L . -la -lb -o test && ./test
#include <cassert>
void const* a();
void const* b();
int main() { assert(a() == b()); }
EOFThe above will fail because the two dylibs were compiled with -fvisibility=hidden. Since we don't annotate std::is_integral_v with any visibility annotation, it gets the visibility set by -fvisibility=hidden, so it is hidden. Both liba.dylib and libb.dylib get a definition of std::is_integral_v (that is the case regardless of -fvisibility=hidden). However, when we run the test program, the dynamic linker doesn't see that both liba.dylib and libb.dylib are using the same std::is_integral_v, because it has hidden visibility in both of them (technically only one of the two needs hidden visibility). As a result, both a() and b() return addresses to different objects and the assertion fails. Now, if you go ahead and remove -fvisibility=hidden from the command lines above, suddenly liba.dylib and libb.dylib start exporting std::is_integral_v and the dynamic linker de-duplicates those at load time. And the assertion passes. So, do we want this to work (knowing there is a load time and symbol table size impact)? In my opinion, the pragmatic thing is to say that it shouldn't work, in which case we'd go ahead and mark those inline variables as _LIBCPP_HIDE_FROM_ABI. However, since we don't do that right now, I'd suggest you hold off on doing it and we can apply it throughout the library. Actually if someone wanted to do that, it would definitely solve one of my problems right now :-) | |
| libcxx/test/std/input.output/filesystems/fs.filesystem.synopsis/enable_borrowed_range.compile.pass.cpp | ||
| 15–18 | Here and elsewhere. | |
| libcxx/include/filesystem | ||
|---|---|---|
| 3024 | If I understand your program correctly, then yes, we not only "want" it to work, but it's mandated by the Standard to work. When someone takes the address of std::is_integral_v<int> from multiple TUs, they must see the same address from both TUs, because the Standard says so (via the inline keyword). Arguably, as soon as the user passes -fvisibility=hidden -shared, all bets are off: is that what we're claiming here? Re _LIBCPP_TEMPLATE_VIS, ignore me; I think I was looking at something like less<void> when I said that. Which is a full specialization, but of a type, not a variable. | |
| libcxx/include/filesystem | ||
|---|---|---|
| 3024 | We're not talking about two different TUs here, we're talking about two different shared libraries. But that's orthogonal to the issue we're discussing because what should work across TUs should also work across dylib boundaries. However, I think you made me change my mind here. I was going to quote http://eel.is/c++draft/constraints to say how users are not allowed to depend on the address of those variable templates, however it appears that we only prohibit that for functions in namespace std, not for variables. So indeed, it seems like users are allowed to expect that the above will work, and so throwing _LIBCPP_HIDE_FROM_ABI onto all those variable templates would make us non-conforming. Ugh, that's a nasty problem cause I don't think the intent was ever for the address of those variable templates to be relevant. | |
"|Complete|","14.0" for consistency with other lines (e.g. line 73)