[libcxx-commits] [PATCH] D118736: [libc++] Guard std::ranges under _LIBCPP_HAS_NO_INCOMPLETE_RANGES.

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

Quuxplusone added a comment.

In D118736#3295144 <https://reviews.llvm.org/D118736#3295144>, @ldionne wrote:

> I am somewhat worried about this. On the one hand this is the "correct" thing to do, however I am concerned that this could actually end up removing some features we've shipped in LLVM 13 (accidentally), and hence break users.

Yup.

> So far, I get the feeling that we've (mistakenly) used `_LIBCPP_HAS_NO_INCOMPLETE_RANGES` as a proxy for something closer to "no ranges functions and types", not for "nothing in the ranges namespace". So, for example, we've been happily providing concepts that are related to ranges (which as a result makes it possible to provide stuff like `span` constructors). I suspect we'd have to touch way fewer files if we still allowed a couple of ranges-related functionality to creep in despite `_LIBCPP_HAS_NO_INCOMPLETE_RANGES` being defined, but I am having trouble defining what should and should not be guarded precisely.

Also yup. I think if you're worried about ABI issues, then it might make sense to restrict the disabled stuff to //just// the view adaptors — e.g. guard `ranges::reverse_view`, but don't guard `ranges::distance` because although the //behavior// of `ranges::distance` will change, the //struct layout// will not (it'll always be an empty type).

> I'm not really sure how to cut the cake, but perhaps I'd try to only remove:
>
> - the content in `__algorithm`, `__filesystem`, `__functional`

The content in `<filesystem>` is just opting into view-ness and/or borrowed-ness for certain existing types. I think that's harmless. The //only// reason I disabled it in this PR right now is because my chosen criterion was "When the macro is present then `namespace ranges` is //not// present, period."

The content in `<functional>`, i.e. `ranges::less` etc., is also stateless empty types, like `ranges::distance`, where again it's quite possible that their //implementation// will change but their //struct layout// definitely won't. So if (if!) our criterion is going to be "enable the empty types and disable the non-empty types," then `ranges::less` should be enabled, as should all of the algorithms we've implemented so far (which maybe is the empty set ;)).

Is this kind of what you had in mind with "concrete views" and "concrete types" — basically a criterion of "any non-empty type should be guarded by the macro"?

> In particular, that should leave us with most of the concepts defined unless I'm mistaken. And those concepts should be reasonable stable AFAICT, and they will enable most of the important stuff like `span` and `string_view`. What do you think?

I think certain concepts, like `output_iterator`, are known to be unstable because there's already changes coming in C++23 and/or late-breaking C++20. (Unless all the changes have already come? Aren't there still major changes in flight re: move-only iterator concepts?) But since concepts don't affect struct layout, maybe we don't care about them from an ABI perspective. I'm still //extremely// hazy on why we're guarding any of this stuff — what failure mode we're afraid of.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118736