The ranges concepts were already available in libc++13, so we shouldn't guard them with _LIBCPP_HAS_NO_INCOMPLETE_RANGES.
Fixes https://github.com/llvm/llvm-project/issues/54765
Details
- Reviewers
Mordante var-const ldionne - Group Reviewers
Restricted Project - Commits
- rGb177a90ce7b5: [libc++] Always enable the ranges concepts
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This change was introduced in D118736, did you verify whether other part of the commit need to be reverted?
Did you verify whether there are unit tests that need to be reenabled?
(The packagers test the release branch so it would be good to have it there too.)
libcxx/include/__ranges/concepts.h | ||
---|---|---|
71 | When removing this, please add some comment at the top of the file why this part of ranges isn't guarded by _LIBCPP_HAS_NO_INCOMPLETE_RANGES |
@Mordante There are quite a few things that were shipped in libc++13 but are guarded now under _LIBCPP_HAS_NO_INCOMPLETE_RANGES. So I'm not certain anymore that we should re-enable the concepts. Do you have any thoughts? https://github.com/llvm/llvm-project/tree/release/13.x/libcxx/include/__ranges is the full list of ranges things that were shipped in libc++13. All the views and data, size etc. are now guarded.
Note in libc++13 we had a guard #if !defined(_LIBCPP_HAS_NO_INCOMPLETE_RANGES) in the ranges header. This is the same way I've guarded the format header.
There's probably a difference for apple-clang and normal clang since that version of apple-clang had no concepts support so there I expect the code was never enabled.
We changed to a more frequent release schedule for the LLVM 14 branch (https://discourse.llvm.org/t/llvm-14-0-1-schedule-and-release-updates/61227/20). I would propose bring this to @ldionne's attention when he's back. Then, if needed, we can aim to have a fix in LLVM 4.0.3, slated for the 10th of May. I'm not sure what the best approach is, I don't know what the impact of the change is for our vendors.
D118736 deliberately made all the contents of the ranges namespace guarded by the macro, so I wouldn't undo that change lightly. It is a very valid argument that a user was broken by that patch. On the other hand, the main reason behind adding the macro was that we don't want users to start relying on ranges while the implementation is in progress and can (at least potentially) change in non-compatible ways. Let's wait for @ldionne to come back and discuss this patch then.
According to https://github.com/llvm/llvm-project/issues/54765, we have until the end of May to ship this (if we decide to ship this) and still make it into LLVM 14.
If we do this, we'll have to also un-guard other things like ranges::data and more (see the failures in https://buildkite.com/llvm-project/libcxx-ci/builds/10285#6807f5d7-5e60-40fa-9fa9-c8736f986f8d).
I think it would be reasonable to do this: the goal of _LIBCPP_HAS_NO_INCOMPLETE_RANGES was primarily to prevent users from depending on ABI/API-unstable things, but these concepts probably won't change in the future.
Let's see what's required to fix the CI and then ship this on main, and cherry-pick onto releases/14.x before the next 14.x release.
@philnik This bug seems to be related https://github.com/llvm/llvm-project/issues/54765. It's not a concept but also used to work.
Ship it. Please ping me when this is committed and I will cherry-pick to the LLVM 14 release branch.
When removing this, please add some comment at the top of the file why this part of ranges isn't guarded by _LIBCPP_HAS_NO_INCOMPLETE_RANGES