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

Restrict ADL for reference_wrapper and thread #3101

Merged
merged 4 commits into from Oct 12, 2022

Conversation

frederick-vs-ja
Copy link
Contributor

This PR is unblocking two libcxx tests.

  • std/thread/thread.threads/thread.thread.class/thread.thread.constr/robust_against_adl.pass.cpp
  • std/utilities/function.objects/refwrap/refwrap.invoke/robust_against_adl.pass.cpp

Driven-by changes:

  • <filesystem>: path's comparison operators are IF-NDR #2358 is fixed, but the related libcxx test still fails because it requires path's iterator to satisfy bidirectional_iterator. The explanation should be changed.
  • Referenced number to the Working Draft in <type_traits> and <thread> are updated to WG21-N4917 with section numbers added.
  • Other ADL-related failures get analyzed.

For `reference_wrapper` and `thread`. And update the references to the latest Working Draft as driven-by change.
@frederick-vs-ja frederick-vs-ja requested a review from a team as a code owner September 14, 2022 08:26
@strega-nil-ms strega-nil-ms added the decision needed We need to choose something before working on this label Sep 14, 2022
@strega-nil-ms
Copy link
Contributor

Marking decision needed, as we need to discuss the underlying issue #140 this week at the STL maintainers' meeting.

@strega-nil-ms strega-nil-ms added this to Initial Review in Code Reviews via automation Sep 14, 2022
@StephanTLavavej StephanTLavavej added bug Something isn't working libcxx skip and removed decision needed We need to choose something before working on this libcxx skip labels Sep 21, 2022
@StephanTLavavej
Copy link
Member

Removing decision needed (as we've decided in #140), and this PR doesn't actually involve qualifying _Ugly names.

@frederick-vs-ja
Copy link
Contributor Author

Removing decision needed (as we've decided in #140), and this PR doesn't actually involve qualifying _Ugly names.

Oh, I'm sorry for doing too much in one commit.😹 This PR does _STD-qualify _Refwrap_ctor_fun (only this _Ugly name).

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me! Thanks!

stl/inc/thread Outdated Show resolved Hide resolved
@strega-nil-ms strega-nil-ms moved this from Initial Review to Final Review in Code Reviews Sep 22, 2022
stl/inc/thread Outdated Show resolved Hide resolved
stl/inc/thread Outdated Show resolved Hide resolved
stl/inc/type_traits Outdated Show resolved Hide resolved
stl/inc/type_traits Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Sep 23, 2022

Thanks - I've pushed small changes to fix citations, clarify a comment, and remove unnecessary parens. FYI @strega-nil-ms (as you previously approved) and @CaseyCarter (if you want to double-check the citation that you added in 2019).

✅ No modules impact.

@StephanTLavavej StephanTLavavej moved this from Final Review to Ready To Merge in Code Reviews Sep 23, 2022
@CaseyCarter
Copy link
Member

I'm not enthusiastic about section numbers in our working draft citations - they're just noise - but it's not worth the effort to pull them out at this point.

@frederick-vs-ja
Copy link
Contributor Author

I'm not enthusiastic about section numbers in our working draft citations - they're just noise - but it's not worth the effort to pull them out at this point.

I can go with removal of section numbers. Should we make an issue to track this?

@StephanTLavavej StephanTLavavej self-assigned this Oct 11, 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 6128830 into microsoft:main Oct 12, 2022
Code Reviews automation moved this from Ready To Merge to Done Oct 12, 2022
@StephanTLavavej
Copy link
Member

Thanks for investigating and fixing these libcxx test failures! 🚀 😻 📉

@StephanTLavavej
Copy link
Member

@frederick-vs-ja We already had #182, so I updated that issue to mention that section numbers should be dropped.

@frederick-vs-ja frederick-vs-ja deleted the small-adl-cleanup branch October 12, 2022 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants