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

Fix VS static analyzer warnings. #1235

Merged
merged 2 commits into from
Oct 26, 2023

Conversation

BillyONeal
Copy link
Member

  1. Optional and expected were not properly forwarding noexcept for their special member functions, so is_nothrow_move_constructible_v<Expected> was false for all T. (This has bad implications if the expecteds are in a vector)
  2. system.process.cpp uses a 32k stack buffer but it's the "top" of the stack so we don't care.
  3. dependencies.cpp was calling std::move() on a constant variable info_ptr->default_features; I changed the declarations to better show that this was const.
  4. (Drive by) All users of make_optional were actually doing emplacement.

1. Optional and expected were not properly forwarding noexcept for their special member functions, so is_nothrow_move_constructible_v<Expected<T>> was false for all T. (This has bad implications if the expecteds are in a vector)
2. system.process.cpp uses a 32k stack buffer but it's the "top" of the stack so we don't care.
3. dependencies.cpp was calling std::move() on a constant variable `info_ptr->default_features`; I changed the declarations to better show that this was const.
4. (Drive by) All users of make_optional were actually doing emplacement.
@Thomas1664
Copy link
Contributor

Thanks for fixing this! I noticed an exceptionally high number of copies out of Expected/ Optional in the past but was unable to find the reason for this.

@BillyONeal
Copy link
Member Author

Thanks for fixing this! I noticed an exceptionally high number of copies out of Expected/ Optional in the past but was unable to find the reason for this.

I doubt this is going to change much about that but it's worth a shot

@@ -48,25 +56,28 @@ namespace vcpkg
}
}

~OptionalStorage() noexcept
~OptionalStorage()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should this not be noexcept?

Copy link
Member Author

@BillyONeal BillyONeal Oct 26, 2023

Choose a reason for hiding this comment

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

It is a destructor, destructors are noexcept without being annotated as such (And we don't annotate other destructors)

Destructors should get noexcept annotations only if they are doing noexcept(false)

@BillyONeal BillyONeal merged commit e0ec6cd into microsoft:main Oct 26, 2023
5 checks passed
@BillyONeal BillyONeal deleted the throwing-expected-error branch October 26, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants