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

Cleanups for <exception> #3973

Merged

Conversation

achabense
Copy link
Contributor

@achabense achabense commented Aug 17, 2023

  • Merge _Current_exception
  • Move _Throw_bad_array_new_length downstream to <xmemory>
  • Simplify _With_nested

There were some attempts to do cleanups for <variant> as well; all of them turned out to be invalid however. Thanks to @frederick-vs-ja and @cpplearner for pointing out the problems :(

@achabense achabense requested a review from a team as a code owner August 17, 2023 03:26
@achabense
Copy link
Contributor Author

achabense commented Aug 17, 2023

STL/stl/inc/exception

Lines 322 to 323 in 8674b3d

template <class _Ex>
void* __GetExceptionInfo(_Ex);

By searching in github I realize this is a builtin. Other projects typically make comments for it (search). I think for clarity we should add a comment as well. (Also, is it ok to add noexcept? It is called by noexcept make_exception_ptr.)

@achabense

This comment was marked as resolved.

stl/inc/format Outdated Show resolved Hide resolved
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Aug 17, 2023
@achabense
Copy link
Contributor Author

achabense commented Aug 18, 2023

_STD _Throw_bad_variant_access();

Is _Throw_bad_variant_access in join_with_view::_Iterator's methods meaningful? It seems this will only happen when the iterator becomes broken due to exception from _Inner_it._Emplace_first / second in previous calls. Even though _Inner_it is a _Variantish, I think bad_variant_access will be very surprising for users of join_with_view.

-- update --
hmm
#2619 (comment)

-- update --
Still, is it possible to replace bad_variant_access exception with something else, like

[[noreturn]] static void _Xbroken_iterator() {
      _Xinvalid_argument("the iterator has been broken due to an exception");
}

If this makes sense, we can move bad_variant_access back to <variant>.

(I think we can also add [[unlikely]] for these _Variantish_state::_Nothing branches.)

@achabense
Copy link
Contributor Author

achabense commented Aug 30, 2023

If this makes sense, we can move bad_variant_access back to .

Roughly like this: a9190c7; is this a valid approach?

@cpplearner
Copy link
Contributor

I don't really care how a valueless_by_exception iterator should behave, but if we decide to not throw bad_variant_access (thus not equivalent to get and visit), we should probably ask LWG to make it standard.

@achabense
Copy link
Contributor Author

achabense commented Aug 30, 2023

Hmm, I wasn't aware the methods should behave as if operating on a real variant.

... So there is nothing changeable about <variant> in this pr.

@achabense achabense changed the title Cleanups for <variant> and <exception> Cleanups for <exception> Aug 30, 2023
@achabense
Copy link
Contributor Author

achabense commented Aug 31, 2023

@CaseyCarter I made a new push to replace _THROW(format_error(...)) with widely-applied _Throw_format_error(...) in <chrono>. (And I begin to feel uncertain about the relocation of _Throw_bad_array_new_length. It seems _Throw_something is often defined closely after the definition of something; though bad_array_new_length is special as it is conditionally defined there...)

@CaseyCarter
Copy link
Member

(And I begin to feel uncertain about the relocation of _Throw_bad_array_new_length. It seems _Throw_something is often defined closely after the definition of something; though bad_array_new_length is special as it is conditionally defined there...)

I suppose it's a question of whether we consider _Throw_meow to be a local helper function that should be defined near its uses, or part of the API of meow which should be defined near the definition of meow. Maybe we should (in a separate change) go all-in on "part of the API" and make them static member functions meow::_Throw?

@StephanTLavavej StephanTLavavej self-assigned this Sep 2, 2023
@StephanTLavavej StephanTLavavej removed their assignment Sep 6, 2023
@StephanTLavavej StephanTLavavej self-assigned this Sep 7, 2023
@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 d16d735 into microsoft:main Sep 7, 2023
35 checks passed
@StephanTLavavej
Copy link
Member

🧹 ⚠️ 😻

@achabense achabense deleted the _Cleanups_variant_and_exception branch September 8, 2023 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Something can be improved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants