Skip to content
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.

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

add test coverage for P2445R1 (std::forward_like) #3072

Merged
merged 1 commit into from
Sep 3, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Sep 2, 2022

While adding std::forward_like from MSVC STL to libc++, we add more checks: https://reviews.llvm.org/D132327

  1. Clang has a new warning if std::move used without std

  2. Check constexpr bool test() in runtime too, not only compile time.

  3. Add all combinations of const and && for constexpr bool test()

  4. Add a test to ensure that std::forward_like doesn't use copy/ctor/default constructors

  5. Add a test with the same type.

@fsb4000 fsb4000 requested a review from a team as a code owner September 2, 2022 16:03
@CaseyCarter CaseyCarter added the test Related to test code label Sep 2, 2022
@CaseyCarter CaseyCarter added this to Initial Review in Code Reviews via automation Sep 2, 2022
static_assert(is_same_v<decltype(forward_like<T>(CU{})), CU&&>);
static_assert(is_same_v<decltype(forward_like<T>(u)), U&&>);
static_assert(is_same_v<decltype(forward_like<T>(cu)), CU&&>);
static_assert(is_same_v<decltype(forward_like<T>(std::move(u))), U&&>);
Copy link
Member

@CaseyCarter CaseyCarter Sep 2, 2022

Choose a reason for hiding this comment

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

Note to other reviewers: I'm not requesting a comment clarifying why we qualify std::move throughout despite using namespace std. Yes, it's unconventional, but I assume this usage will become widespread when we adopt Clang 15 with the new warning about using move unqualified. (No change requested.)

Copy link
Member

Choose a reason for hiding this comment

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

This warning might be obnoxious/widespread enough that we'll need to silence it in the matrices. (I'm not too worried about omitting _STD in product code, we're pretty good about that.) Still, no objections to doing this for now.

@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in Code Reviews Sep 2, 2022
@StephanTLavavej StephanTLavavej self-assigned this Sep 3, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit f9697fc into microsoft:main Sep 3, 2022
Code Reviews automation moved this from Ready To Merge to Done Sep 3, 2022
@StephanTLavavej
Copy link
Member

Thanks for enhancing this test coverage and preventing future bugs! 🐞 ✅ 😹

@fsb4000 fsb4000 deleted the forward_like_tests branch September 4, 2022 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Related to test code
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants