Skip to content

update the feature-test macro for P2162R2 for variant #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Jun 29, 2021
Merged

update the feature-test macro for P2162R2 for variant #2006

merged 12 commits into from
Jun 29, 2021

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Jun 13, 2021

Fixes #1683

@fsb4000 fsb4000 requested a review from a team as a code owner June 13, 2021 06:19
@SuperWig
Copy link
Contributor

You need to also update the feature test macro test.

@SuperWig
Copy link
Contributor

Come to think of it. Considering this was implemented in C++17, thus available in that mode, shouldn't the value just be updated instead of under _HAS_CXX23?

@fsb4000
Copy link
Contributor Author

fsb4000 commented Jun 13, 2021 •
edited
Loading

Yes. I found it when inspected failed test: "std/language.support/support.limits/support.limits.general/variant.version.pass.cpp"
llvm/llvm-project@0324b46#diff-a9be5c3f8e18be99f40fb35f58960f400413a76252d86b53f80d76cb09cb53ef

@AdamBucior
Copy link
Contributor

Do we have test coverage for that to ensure we never accidentally degrade it?

@StephanTLavavej StephanTLavavej added the cxx23 C++23 feature label Jun 14, 2021
CaseyCarter
CaseyCarter suggested changes Jun 21, 2021
View reviewed changes
StephanTLavavej
StephanTLavavej reviewed Jun 23, 2021
View reviewed changes
Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'll push a couple of trivial test changes for style/comment issues I noticed.

@StephanTLavavej StephanTLavavej self-assigned this Jun 24, 2021
@StephanTLavavej StephanTLavavej merged commit aaa5d08 into microsoft:main Jun 29, 2021
@StephanTLavavej
Copy link
Member

Thanks for looking into this and adding test coverage - one step closer to C++23 completeness! 🎉 😸 ✔️

@fsb4000 fsb4000 deleted the 1683 branch June 29, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+1 more reviewer

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@StephanTLavavej StephanTLavavej

Labels
cxx23 C++23 feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P2162R2 Inheriting From variant
5 participants
@fsb4000 @SuperWig @AdamBucior @StephanTLavavej @CaseyCarter