Skip to content

Add Non-exception Throwing Specifier to ranges::ssize#4231

Merged
StephanTLavavej merged 3 commits intomicrosoft:mainfrom
PhiliaTheCat:PhiliaTheCat/issue4107
Dec 7, 2023
Merged

Add Non-exception Throwing Specifier to ranges::ssize#4231
StephanTLavavej merged 3 commits intomicrosoft:mainfrom
PhiliaTheCat:PhiliaTheCat/issue4107

Conversation

@PhiliaTheCat
Copy link
Contributor

@PhiliaTheCat PhiliaTheCat commented Dec 2, 2023 •
edited by StephanTLavavej
Loading

Fixes #4107.

Non-exception throwing specifier added in stl/inc/xutility:3357

Besides, some improper fomats are changed in stl/inc/xutility

@PhiliaTheCat PhiliaTheCat requested a review from a team as a code owner December 2, 2023 02:40
frederick-vs-ja
frederick-vs-ja reviewed Dec 2, 2023
View reviewed changes
Copy link
Contributor

@frederick-vs-ja frederick-vs-ja left a comment

Choose a reason for hiding this comment

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

It would be better to use GitHub's close/fix/resolve syntax (e.g. saying Fixes #4107) in the description of this PR (instead of the commit message).

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Dec 2, 2023

Also, it would be better to add STATIC_ASSERT(noexcept(ranges::ssize(t)) == Nothrow); immediately below this line, which would properly test the corrected exception specification.

STL/tests/std/tests/P0896R4_ranges_range_machinery/test.cpp

Line 1531 in 0403d19

STATIC_ASSERT(noexcept(ranges::size(t)) == Nothrow);

@PhiliaTheCat
Copy link
Contributor Author

PhiliaTheCat commented Dec 2, 2023

@microsoft-github-policy-service agree

@StephanTLavavej StephanTLavavej added bug Something isn't working ranges C++20/23 ranges labels Dec 3, 2023
@StephanTLavavej StephanTLavavej changed the title Fixes #4107: Add Non-exception Throwing Specifier Add Non-exception Throwing Specifier to ranges::ssize Dec 3, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Dec 3, 2023

Looks great, thanks! I updated your PR description to properly link to the issue that it resolves, and updated the PR title to mention what ranges function is being changed.

I think this needs only one maintainer approval. We merge PRs simultaneously to the GitHub and MSVC-internal repos, batched up in a semi-manual process to save time. Your PR will be part of the next batch, probably next week.

@StephanTLavavej StephanTLavavej self-assigned this Dec 7, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Dec 7, 2023

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

@PhiliaTheCat
Copy link
Contributor Author

PhiliaTheCat commented Dec 7, 2023 via email

@StephanTLavavej StephanTLavavej merged commit 706b425 into microsoft:main Dec 7, 2023
@StephanTLavavej
Copy link
Member

StephanTLavavej commented Dec 7, 2023

Thanks for fixing ranges::ssize and congratulations on your first microsoft/STL commit! 🎉 😻 🚀

This will ship in VS 2022 17.10 Preview 1.

@PhiliaTheCat PhiliaTheCat deleted the PhiliaTheCat/issue4107 branch December 8, 2023 05:53
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

+2 more reviewers

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

@CaseyCarter CaseyCarter CaseyCarter approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

bug Something isn't working ranges C++20/23 ranges

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

<xutility>: ranges::_Ssize_fn::operator() should have a noexcept-specifier

4 participants

@PhiliaTheCat @frederick-vs-ja @StephanTLavavej @CaseyCarter