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.

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

<tuple>: Make std::make_from_tuple SFINAE friendly #4528

Merged
merged 28 commits into from
Mar 28, 2024
Merged

<tuple>: Make std::make_from_tuple SFINAE friendly #4528

merged 28 commits into from
Mar 28, 2024

Conversation

yronglin
Copy link
Contributor

STL now use static_assert to implements LWG3528, but after discussion in llvm/llvm-project#85263, we can constraint std::make_from_tuple and std::_Make_from_tuple to make them SFINAE friendly.

@yronglin yronglin requested a review from a team as a code owner March 24, 2024 09:22
@yronglin
Copy link
Contributor Author

@microsoft-github-policy-service agree

yronglin added 3 commits March 24, 2024 17:52
@yronglin
Format
731c998 
Signed-off-by: yronglin <[email protected]>
@yronglin
Format
959808d 
Signed-off-by: yronglin <[email protected]>
@yronglin
Revert incorrect change
e41ba98 
Signed-off-by: yronglin <[email protected]>
frederick-vs-ja
frederick-vs-ja reviewed Mar 24, 2024
View reviewed changes
@yronglin
Copy link
Contributor Author

Thanks for your review! @frederick-vs-ja

@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Mar 25, 2024
@StephanTLavavej StephanTLavavej self-assigned this Mar 25, 2024
@yronglin
Copy link
Contributor Author

Thanks for your review! @AlexGuteniev

@StephanTLavavej
Add new test to tests/std/test.lst.
13a6ab6
@StephanTLavavej
Mention libc++'s test files.
f7ff18c
@StephanTLavavej
Add std:: qualification to <cstdint> types.
ab8644f
@StephanTLavavej
Style: Use empty braces for std::uint8_t{} temporaries.
3073394
@StephanTLavavej
Directly construct make_index_sequence<MEOW>{}, no need for declval.
ab20411
@StephanTLavavej
Rename _Ugly identifiers: _Ty => T
26ef5d2
@StephanTLavavej
Rename _Ugly identifiers: _Tuple => Tuple
d65d9e6
@StephanTLavavej
Rename _Ugly identifiers: _Indices => Indices
7c99e8b
@StephanTLavavej
Rename _Ugly identifiers: _Seq => Seq
a5b121a
@StephanTLavavej
Nitpick: struct inheritance is already public by default.
fd217a5
@StephanTLavavej
Avoid potentially-confusing && in comments.
1d9bdad
@StephanTLavavej
Fix comment typo (extra underscore).
48f81d6
@StephanTLavavej
B and C were unused.
9146dfb
@StephanTLavavej
Drop old-style (function-based) SFINAE test coverage.
920c5d6 
Verified that this exactly duplicated the new-style (partial specialization) coverage.
@StephanTLavavej
No longer need <cstdint> and "partial specialization" comments.
37b1319
StephanTLavavej
StephanTLavavej reviewed Mar 26, 2024
View reviewed changes
@StephanTLavavej StephanTLavavej changed the title <tuple>: Make std::make_from_tuple and std::_Make_from_tuple_impl SFINAE friendly <tuple>: Make std::make_from_tuple SFINAE friendly Mar 26, 2024
@StephanTLavavej StephanTLavavej removed their assignment Mar 26, 2024
@StephanTLavavej
Copy link
Member

Thanks! 😻 I pushed a bunch of fine-grained commits, simplifying the product and test code. (The commits correspond to each of my comments as I discovered them, and the tests should pass at each point; I intentionally didn't clean up the history even though later commits ended up making some of the earlier commits moot.) Please meow if I messed anything up!

@yronglin
Copy link
Contributor Author

Thanks! 😻 I pushed a bunch of fine-grained commits, simplifying the product and test code. (The commits correspond to each of my comments as I discovered them, and the tests should pass at each point; I intentionally didn't clean up the history even though later commits ended up making some of the earlier commits moot.) Please meow if I messed anything up!

Thanks for your review and help, these changes all looks good to me. 🥳

@yronglin
Copy link
Contributor Author

Thank you all for your such detailed comments. I learned a lot of new knowledge from your comments! 🤩

@StephanTLavavej StephanTLavavej self-assigned this Mar 27, 2024
@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 ece6dd3 into microsoft:main Mar 28, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for improving this tuple factory! 🏭 📦 📦 📦

@frederick-vs-ja
Copy link
Contributor

Thans for refixing my first fix (#2160) in MSVC STL!

@StephanTLavavej StephanTLavavej added bug Something isn't working and removed enhancement Something can be improved labels Mar 28, 2024
@yronglin
Copy link
Contributor Author

yronglin commented Apr 1, 2024

Thans for refixing my first fix (#2160) in MSVC STL!

It's my pleasure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexGuteniev AlexGuteniev AlexGuteniev left review comments

@frederick-vs-ja frederick-vs-ja frederick-vs-ja left review comments

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

Assignees

@StephanTLavavej StephanTLavavej

Labels
bug Something isn't working
Projects
STL Code Reviews
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yronglin @StephanTLavavej @frederick-vs-ja @AlexGuteniev