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

Miscellaneous Cleanups #3178

Merged
merged 10 commits into from
Oct 26, 2022
Merged

Miscellaneous Cleanups #3178

merged 10 commits into from
Oct 26, 2022

Conversation

CaseyCarter
Copy link
Member

Mostly things I noticed while reviewing the last import batch at the last minute that were not worth resetting testing for, but does include a couple of "old" changes as well.

Separate commits to ease review:

  • "basic_string::_Move_construct_from_substr already knows the allocator": avoids passing an allocator to this member function which can simply retrieve the stored allocator.
  • "Consistently guard _Verify_offset(-n) against -INT_MIN" (in <ranges>): we had a couple of places that guarded against overflow negating the argument to _Verify_offset, we should do so consistently.
  • "Cleanup _NOEXCEPT_IDL0": Removes this variation in noexcept between debug and release builds - it was a terrible idea. Make transform_view::iterator::_Verify_offset always noexcept by avoiding checks when the underlying iterator doesn't support _Verify_offset. Make that function and elements_view::iterator::_Verify_offset available only when _IDL != 0 fixing a bug in elements_view::iterator that disabled the check. Remove the now unused _NOEXCEPT_IDL0 machinery. Fixes <ranges>: Questionable conditional noexcept #1269.
  • "Keep type and data member definitions together in <execution>": Fixes <execution>: Consider separating type and data member declarations #330.
  • "<system_error>: Correct if (!cond) x(); else y()": ... to if (cond) y(); else x();. Drive-by: Let's static_assert only when running the test suite.
  • "tests/P2278R4_basic_const_iterator nitpicks":
    • Simplify nested requirement in HasPeek
    • Remove excess empty line
  • "tests/P2322R6_ranges_alg_fold: factor out non-dependent tests": ... from the dependent test cases in instantiator::call. Reduces compile + run time by 27%.

Make `transform_view::iterator::_Verify_offset` always `noexcept` by avoiding checks when the underlying iterator doesn't support `_Verify_offset`. Make that function and `elements_view::iterator::_Verify_offset` available only when `_IDL != 0` **fixing a bug in `elements_view::iterator`**. Remove the now unused `_NOEXCEPT_IDL0` machinery.

Fixes microsoft#1269
... to `if (cond) y(); else x();`

Drive-by: Let's `static_assert` only when running the test suite.
* Simplify nested requirement in `HasPeek`
* Remove excess empty line
... from `instantiator::call`. Reduces compile + run time by 27%.
@CaseyCarter CaseyCarter requested a review from a team as a code owner October 25, 2022 17:31
@CaseyCarter CaseyCarter added the bug Something isn't working label Oct 25, 2022
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.

LGTM! (obviously, fix the red cross)

stl/inc/system_error Outdated Show resolved Hide resolved
@StephanTLavavej
Copy link
Member

I've pushed a commit to fix the test failures because _STL_INTERNAL_STATIC_ASSERT doesn't take a reason (maintainers are expected to know the reason through their psychic debugging powers 😹).

Additionally, I split it into two static assertions, partially to make things line up nicer, but also to follow our usual guidance to assert X and Y separately instead of X && Y (so if it ever fails, we can immediately see why), even though this is admittedly at very low risk of ever failing.

transform_view iterators are validated only when the underlying iterators are.
@CaseyCarter

This comment was marked as resolved.

@azure-pipelines

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej self-assigned this Oct 25, 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 e97bb2b into microsoft:main Oct 26, 2022
@StephanTLavavej
Copy link
Member

Thanks for finding and fixing these issues! 🏚️ 🧹 🏡

@CaseyCarter CaseyCarter deleted the misc branch October 26, 2022 17:49
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
None yet
3 participants