[libcxx-commits] [PATCH] D118507: [libc++] Merge _LIBCPP_HAS_NO_RANGES into _LIBCPP_HAS_NO_CONCEPTS. NFC

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__config:882
 
-#if _LIBCPP_STD_VER <= 17 || defined(_LIBCPP_HAS_NO_CONCEPTS)
-#define _LIBCPP_HAS_NO_RANGES
----------------
Quuxplusone wrote:
> Mordante wrote:
> > 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.
> 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?
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118507