This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Always enable the ranges concepts
ClosedPublic

Authored by philnik on Apr 19 2022, 9:07 AM.

Details

Reviewers
Mordante
var-const
ldionne
Group Reviewers
Restricted Project
Commits
rGb177a90ce7b5: [libc++] Always enable the ranges concepts
Summary

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

Diff Detail

Event Timeline

philnik created this revision.Apr 19 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 9:07 AM
philnik requested review of this revision.Apr 19 2022, 9:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 19 2022, 9:07 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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.

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

ldionne requested changes to this revision.Apr 25 2022, 9:40 AM

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.

This revision now requires changes to proceed.Apr 25 2022, 9:40 AM
philnik updated this revision to Diff 428931.May 12 2022, 6:48 AM
  • try to fix CI

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

ldionne accepted this revision.May 23 2022, 10:26 AM

Ship it. Please ping me when this is committed and I will cherry-pick to the LLVM 14 release branch.

This revision is now accepted and ready to land.May 23 2022, 10:26 AM
This revision was landed with ongoing or failed builds.May 23 2022, 11:45 AM
This revision was automatically updated to reflect the committed changes.