This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Merge _LIBCPP_HAS_NO_RANGES into _LIBCPP_HAS_NO_CONCEPTS. NFC
ClosedPublic

Authored by Quuxplusone on Jan 28 2022, 1:00 PM.

Details

Summary
The macro that opts out of `std::ranges::` functionality is called
`_LIBCPP_HAS_NO_INCOMPLETE_RANGES`, and is unrelated to this macro
which is specifically about _compiler_ support for the _syntax_.

The only non-mechanical diff here is in `<__config>`.

Diff Detail

Event Timeline

Quuxplusone requested review of this revision.Jan 28 2022, 1:00 PM
Quuxplusone created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 28 2022, 1:00 PM
libcxx/include/__filesystem/directory_iterator.h
136

@ldionne: Here's an example of something that should probably be guarded under _LIBCPP_HAS_NO_INCOMPLETE_RANGES (instead of _LIBCPP_HAS_NO_CONCEPTS).

libcxx/include/string_view
726

@ldionne: Some or all of these deduction guides should probably be guarded under _LIBCPP_HAS_NO_INCOMPLETE_RANGES, since ranges::contiguous_range (line 727) won't even exist if libc++ isn't providing Ranges facilities.

libcxx/test/support/test_macros.h
246–248

The testing story is a little messed up, but perhaps this should be guarded under #if TEST_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES). (It's unclear what the purpose of this macro is right now. Compare to the other places in libcxx/test/ that have diffs in this PR.)

jloser accepted this revision.Jan 28 2022, 5:33 PM

LGTM! Thanks for cleaning this up.

I'd be in favor of a patch that guards the enable_view for directory_iterator and recursive_directory_iterator with _LIBCPP_HAS_NO_INCOMPLETE_RANGES instead as you pointed out.

libcxx/include/__filesystem/directory_iterator.h
136

Agreed. I added these IIRC, but copied the #ifdef check from somewhere else likely but never chased down the differences between concepts/ranges checks. I'd be fine if you change this to _LIBCPP_HAS_NO_INCOMPLETE_RANGES.

libcxx/include/__filesystem/recursive_directory_iterator.h
167

Let's also guard this with _LIBCPP_HAS_NO_INCOMPLETE_RANGES as in the directory_iterator case.

Mordante added inline comments.Jan 29 2022, 4:46 AM
libcxx/include/__config
882

How about keeping this and test it for _LIBCPP_HAS_NO_INCOMPLETE_RANGES too? Wouldn't that solve our problem with not testing _LIBCPP_HAS_NO_INCOMPLETE_RANGES everywhere. Of course that would mean we keep both _LIBCPP_HAS_NO_CONCEPTS and _LIBCPP_HAS_NO_RANGES.

Note that adding _LIBCPP_HAS_NO_INCOMPLETE_RANGES was a kind of hurried last minute fix for the LLVM 13 release, but in hindsight updating this part of the __config sounds like a good idea.

libcxx/include/__config
882

I'm not sure I understand, but maybe I do.
We have two orthogonal criteria for #if'ing stuff out right now:

  • Does it (perhaps transitively) depend on the compiler's support for C++20 concept/requires syntax?
  • Does it (perhaps transitively) depend on the vendor's commitment to std::ranges support in the library?

I claim that the former should be called _LIBCPP_HAS_NO_CONCEPTS and the latter should be called _LIBCPP_HAS_NO_INCOMPLETE_RANGES. IIUC, you're saying that it would cause less churn if we also introduced

#if defined(_LIBCPP_HAS_NO_CONCEPTS) || defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES)
 #define _LIBCPP_HAS_NO_RANGES
#endif

So then most of the places guarded by _LIBCPP_HAS_NO_RANGES today would be correct; and probably some of them are wrong, but maybe it would be easier to audit for those (few?) places.

Is that the right idea?

It also occurs to me that it would aid understanding if we renamed s/_LIBCPP_HAS_NO_RANGES/_LIBCPP_HAS_NO_RANGES_NAMESPACE/. That makes it clearer the exact scope we're talking about (more than <ranges>, all of std::ranges; but not including the One Ranges Proposal stuff that lies outside std::ranges, such as std::forward_iterator.) Do we agree on that scope, and if so, do we agree on the renaming?

libcxx/include/__config
882

Actually, what I suggested won't work.

std::forward_iterator depends on std::weakly_incrementable depends on std::movable depends on std::swappable depends on ranges::swap. So there is effectively no way to snip out just namespace ranges without snipping all of the standard concepts as well.

@Mordante: Does this change your suggested approach?

Mordante added inline comments.Jan 30 2022, 6:08 AM
libcxx/include/__config
882

Your previous example was indeed what I suggested. It's unfortunate that this doesn't work, since this basically means it's not possible for vendors to completely opt-out of ranges. Which was the original intention of _LIBCPP_HAS_NO_INCOMPLETE_RANGES. I wonder how useful _LIBCPP_HAS_NO_INCOMPLETE_RANGES is now it's not a proper guard anymore.

Looking at our ranges implementation status there are still quite a number of not implemented papers. I'm not familiar enough with ranges to know whether or not these papers introduce API/ABI-incompatability. So I'm not sure whether we can just remove _LIBCPP_HAS_NO_INCOMPLETE_RANGES or need to guard the unguarded places.

So I prefer to defer the decision of the way forward to @ldionne.

ldionne accepted this revision.Jan 31 2022, 8:48 AM

IMO this looks fine as-is. This is just removing an additional spelling of _LIBCPP_HAS_NO_CONCEPTS by folding it into _LIBCPP_HAS_NO_CONCEPTS. Furthermore, all of these _LIBCPP_HAS_NO_CONCEPTS will be changed to _LIBCPP_STD_VER > 17 as soon as I manage to drop support for a slightly older AppleClang.

Yes, we do have places that are not guarded by _LIBCPP_HAS_NO_INCOMPLETE_RANGES , and we should fix those separately.

Also, personally, I would keep _LIBCPP_HAS_NO_INCOMPLETE_RANGES its own separate macro and use it directly, because we'll mass-remove it as soon as we are sufficiently confident about the stability of <ranges>.

libcxx/include/__filesystem/directory_iterator.h
136

100% agreed, let's fix this.

This revision is now accepted and ready to land.Jan 31 2022, 8:48 AM
This revision was landed with ongoing or failed builds.Jan 31 2022, 9:11 AM
This revision was automatically updated to reflect the committed changes.