Page MenuHomePhabricator

[libc++] LWG3480: make (recursive)_directory_iterator C++20 ranges
ClosedPublic

Authored by jloser on Tue, Oct 12, 6:59 AM.

Details

Summary

Implement LWG3480 which enables directory_iterator and
recursive_directory_iterator to be both a borrowed_range and a
view.

Diff Detail

Event Timeline

jloser requested review of this revision.Tue, Oct 12, 6:59 AM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 12, 6:59 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Quuxplusone requested changes to this revision.Tue, Oct 12, 7:29 AM

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.
The gist is that we're going to pass by value instead of by const-reference.

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.
(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;

*/
2883

Add a test that catches this bug.
Then, remove &.

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.
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.
This revision now requires changes to proceed.Tue, Oct 12, 7:29 AM
jloser updated this revision to Diff 379381.Wed, Oct 13, 7:54 AM
  • 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
jloser marked 7 inline comments as done.Wed, Oct 13, 7:58 AM
jloser added inline comments.
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.

jloser updated this revision to Diff 379387.Wed, Oct 13, 7:59 AM
jloser marked an inline comment as done.

Remove unneeded <concepts> include from enable_view.compile.pass.cpp

Quuxplusone added inline comments.Wed, Oct 13, 9:01 AM
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
13–14

Can we do UNSUPPORTED: libcpp-no-concepts instead? Ditto throughout.

24–25

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

jloser updated this revision to Diff 379477.Wed, Oct 13, 11:23 AM

Test value type, reference type, and const reference types in range concept conformance tests
Fix whitespacing with template<> in synopsis and in tests

jloser updated this revision to Diff 379478.Wed, Oct 13, 11:25 AM

Add _LIBCPP_TEMPLATE_VIS to directory_iterator and recursive_directory_iterator.

jloser marked 3 inline comments as done.Wed, Oct 13, 11:27 AM
jloser added inline comments.
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
13–14

Let's see what BuildKite has to say -- just tried.

jloser updated this revision to Diff 379568.Wed, Oct 13, 5:36 PM

Remove _LIBCPP_TEMPLATE_VIS
Change range_concept_conformance.compile.pass to be UNSUPPORTED: libcpp-has-no-incomplete-ranges

jloser marked an inline comment as done.Wed, Oct 13, 5:39 PM
jloser added inline comments.
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
13–14

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.

ldionne accepted this revision.Thu, Oct 14, 8:20 AM

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()); }
EOF

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

Quuxplusone added inline comments.Thu, Oct 14, 8:43 AM
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.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Oct 14, 9:03 AM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.Thu, Oct 14, 1:56 PM
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.