Skip to content

_STD-qualify _Ugly non-member function calls in - #3148

Merged
Stephan T. Lavavej (StephanTLavavej) merged 3 commits into
microsoft:mainfrom
CaseyCarter:variant_140
Oct 12, 2022
Merged

_STD-qualify _Ugly non-member function calls in #3148
Stephan T. Lavavej (StephanTLavavej) merged 3 commits into
microsoft:mainfrom
CaseyCarter:variant_140

Conversation

@CaseyCarter

@CaseyCarter Casey Carter (CaseyCarter) commented Oct 10, 2022

Copy link
Copy Markdown
Contributor

Qualifies calls to namespace-scope functions in , and enables the libc++ ADL test that now passes.

Partially addresses #140

@CaseyCarter Casey Carter (CaseyCarter) added the enhancement Something can be improved label Oct 10, 2022
@CaseyCarter
Casey Carter (CaseyCarter) requested a review from a team as a code owner October 10, 2022 20:32
@CaseyCarter Casey Carter (CaseyCarter) added bug Something isn't working and removed enhancement Something can be improved labels Oct 10, 2022
@CaseyCarter Casey Carter (CaseyCarter) mentioned this pull request Oct 10, 2022
Open
CaseyCarter
Casey Carter (CaseyCarter) commented Oct 10, 2022
View reviewed changes
Comment thread tests/std/tests/P0088R3_variant/test.cpp Outdated
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Oct 11, 2022

Copy link
Copy Markdown
Member

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

@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Oct 12, 2022

Copy link
Copy Markdown
Member

I've had to push an additional commit to fix internal test failures with /clr. I was a bad kitty and didn't investigate the root cause (or mark this as TRANSITION), but it appears that /clr is deciding to instantiate stuff at the end of the TU, and this ends up requiring completeness. Since this is a fairly niche use case, I figured that just skipping the affected portion for /clr would be strictly better than the status quo where it's unconditionally skipped.

@CaseyCarter

Casey Carter (CaseyCarter) commented Oct 12, 2022

Copy link
Copy Markdown
Contributor Author

I was a bad kitty and didn't investigate the root cause (or mark this as TRANSITION)

I'm the last person you'll have to convince that we should do any extra work where /clr is concerned.

@StephanTLavavej
Stephan T. Lavavej (StephanTLavavej) merged commit cdf9746 into microsoft:main Oct 12, 2022
@StephanTLavavej

Stephan T. Lavavej (StephanTLavavej) commented Oct 12, 2022

Copy link
Copy Markdown
Member

Thanks for investigating and fixing this! 🕵️ 🛠️ 🎉

@CaseyCarter
Casey Carter (CaseyCarter) deleted the variant_140 branch October 12, 2022 19:03
Merged
@frederick-vs-ja A. Jiang (frederick-vs-ja) mentioned this pull request Nov 6, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej) StephanTLavavej approved these changes
+1 more reviewer
@strega-nil-ms nicole mazzuca (strega-nil-ms) strega-nil-ms approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej Stephan T. Lavavej (StephanTLavavej)

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CaseyCarter @StephanTLavavej @strega-nil-ms