This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds __cpp_lib_concepts feature-test macro
ClosedPublic

Authored by cjdb on Apr 2 2021, 11:09 AM.

Details

Summary

Also adjusts C++20 status paper to indicate full concepts support.

Depends on D96477, D99817.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 2 2021, 11:09 AM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 11:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I think it would be best to make this a NFC, so please add the change in libcxx/include/concepts to a different patch with a regression test.

It's also too bad that this relies on D96477. Ideally, we'd land this quickly and then update D96477 to reflect that, but I'm unsure of the timeline, so I'm OK if you want to land D96477 first.

Why do you mark it as complete in version 14? Next release is going to be 13.
And, I agree with Zoe that it should be a NFC commit and the change in <concepts> should be regression tested.

cjdb updated this revision to Diff 335030.Apr 2 2021, 2:37 PM
cjdb edited the summary of this revision. (Show Details)

applies @zoecarver and @curdeius' feedback

I would like to see this patch rebased on main once all it's dependencies have landed. Just to give it a another CI run.
Doesn't finishing P0898R3 mean we should set __cpp_lib_concepts ?
(Note I haven't looked whether all parts of the papers have been implemented.)

cjdb updated this revision to Diff 335171.Apr 4 2021, 2:00 PM
cjdb retitled this revision from [libcxx] adjusts C++20 status paper to indicate full concepts support to [libcxx] adds __cpp_lib_concepts feature-test macro.
cjdb edited the summary of this revision. (Show Details)

enables feature-test macro, revises commit message

Mordante requested changes to this revision.Apr 5 2021, 2:29 AM

enables feature-test macro, revises commit message

Please update utils/generate_feature_test_macro_components.py and run this script. This will properly update the feature-test macros.
Just remove the line "unimplemented": True, for __cpp_lib_concepts.

This revision now requires changes to proceed.Apr 5 2021, 2:29 AM
cjdb updated this revision to Diff 335374.Apr 5 2021, 7:17 PM

properly updates feature-test macro per @Mordante's request

cjdb added a comment.Apr 5 2021, 7:18 PM

@zoecarver @curdeius this patch now changes a feature-test macro, so I don't think it can be NFC. Are you okay with that?

cjdb updated this revision to Diff 335375.Apr 5 2021, 7:19 PM

rebases to fix CI

@zoecarver @curdeius this patch now changes a feature-test macro, so I don't think it can be NFC. Are you okay with that?

If there's doubt, don't mark it as NFC.
Adding NFC when it's not NFC would make more harm than not adding NFC when it really is.

curdeius accepted this revision as: curdeius.Apr 6 2021, 12:36 AM

LGTM. Please rebase to run the CI once the dependencies have landed.

cjdb updated this revision to Diff 335561.Apr 6 2021, 9:54 AM

rebases to activate CI, applies @curdeius' comment suggestions from D96477 (putting them in this patch makes more sense due to this patch's detachment from anything specific)

Mordante accepted this revision.Apr 6 2021, 11:20 AM

LGTM!

This revision is now accepted and ready to land.Apr 6 2021, 11:20 AM
ldionne accepted this revision.Apr 6 2021, 12:27 PM
This revision was automatically updated to reflect the committed changes.