From e0ec6cdb16290826851921f6fa1caefb69211344 Mon Sep 17 00:00:00 2001 From: Billy O'Neal Date: Thu, 26 Oct 2023 13:51:46 -0700 Subject: [PATCH] Fix VS static analyzer warnings. (#1235) * Fix VS static analyzer warnings. 1. Optional and expected were not properly forwarding noexcept for their special member functions, so is_nothrow_move_constructible_v> 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. --- include/vcpkg/base/expected.h | 22 +++--- include/vcpkg/base/optional.h | 120 ++++++++++++++++-------------- src/vcpkg/base/system.process.cpp | 1 + src/vcpkg/dependencies.cpp | 8 +- src/vcpkg/sourceparagraph.cpp | 8 +- 5 files changed, 87 insertions(+), 72 deletions(-) diff --git a/include/vcpkg/base/expected.h b/include/vcpkg/base/expected.h index 682e2af90c..9ead2d49ed 100644 --- a/include/vcpkg/base/expected.h +++ b/include/vcpkg/base/expected.h @@ -53,7 +53,7 @@ namespace vcpkg struct ExpectedHolder { ExpectedHolder() = delete; - ExpectedHolder(T& t) : t(&t) { } + ExpectedHolder(T& t) noexcept : t(&t) { } ExpectedHolder(const ExpectedHolder&) = default; ExpectedHolder& operator=(const ExpectedHolder&) = default; using pointer = T*; @@ -98,7 +98,9 @@ namespace vcpkg { } - ExpectedT(const ExpectedT& other) : value_is_error(other.value_is_error) + ExpectedT(const ExpectedT& other) noexcept( + std::is_nothrow_copy_constructible_v&& std::is_nothrow_copy_constructible_v>) + : value_is_error(other.value_is_error) { if (value_is_error) { @@ -110,7 +112,9 @@ namespace vcpkg } } - ExpectedT(ExpectedT&& other) : value_is_error(other.value_is_error) + ExpectedT(ExpectedT&& other) noexcept( + std::is_nothrow_move_constructible_v&& std::is_nothrow_move_constructible_v>) + : value_is_error(other.value_is_error) { if (value_is_error) { @@ -208,25 +212,25 @@ namespace vcpkg return *m_t.get(); } - const T& value(const LineInfo& line_info) const& + const T& value(const LineInfo& line_info) const& noexcept { unreachable_if_error(line_info); return *m_t.get(); } - T&& value(const LineInfo& line_info) && + T&& value(const LineInfo& line_info) && noexcept { unreachable_if_error(line_info); return std::move(*m_t.get()); } - const Error& error() const& + const Error& error() const& noexcept { unreachable_if_not_error(VCPKG_LINE_INFO); return m_error; } - Error&& error() && + Error&& error() && noexcept { unreachable_if_not_error(VCPKG_LINE_INFO); return std::move(m_error); @@ -358,7 +362,7 @@ namespace vcpkg } private: - void exit_if_error(const LineInfo& line_info) const + void exit_if_error(const LineInfo& line_info) const noexcept { if (value_is_error) { @@ -366,7 +370,7 @@ namespace vcpkg } } - void unreachable_if_error(const LineInfo& line_info) const + void unreachable_if_error(const LineInfo& line_info) const noexcept { if (value_is_error) { diff --git a/include/vcpkg/base/optional.h b/include/vcpkg/base/optional.h index b18a97e602..50a186866d 100644 --- a/include/vcpkg/base/optional.h +++ b/include/vcpkg/base/optional.h @@ -27,10 +27,17 @@ namespace vcpkg struct OptionalStorage { constexpr OptionalStorage() noexcept : m_is_present(false), m_inactive() { } - constexpr OptionalStorage(const T& t) : m_is_present(true), m_t(t) { } - constexpr OptionalStorage(T&& t) : m_is_present(true), m_t(std::move(t)) { } + constexpr OptionalStorage(const T& t) noexcept(std::is_nothrow_copy_constructible_v) + : m_is_present(true), m_t(t) + { + } + constexpr OptionalStorage(T&& t) noexcept(std::is_nothrow_move_constructible_v) + : m_is_present(true), m_t(std::move(t)) + { + } template>> - explicit OptionalStorage(Optional&& t) : m_is_present(false), m_inactive() + explicit OptionalStorage(Optional&& t) noexcept(std::is_nothrow_constructible_v) + : m_is_present(false), m_inactive() { if (auto p = t.get()) { @@ -39,7 +46,8 @@ namespace vcpkg } } template - explicit OptionalStorage(const Optional& t) : m_is_present(false), m_inactive() + explicit OptionalStorage(const Optional& t) noexcept(std::is_nothrow_constructible_v) + : m_is_present(false), m_inactive() { if (auto p = t.get()) { @@ -48,17 +56,19 @@ namespace vcpkg } } - ~OptionalStorage() noexcept + ~OptionalStorage() { if (m_is_present) m_t.~T(); } - OptionalStorage(const OptionalStorage& o) : m_is_present(o.m_is_present), m_inactive() + OptionalStorage(const OptionalStorage& o) noexcept(std::is_nothrow_copy_constructible_v) + : m_is_present(o.m_is_present), m_inactive() { if (m_is_present) new (&m_t) T(o.m_t); } - OptionalStorage(OptionalStorage&& o) noexcept : m_is_present(o.m_is_present), m_inactive() + OptionalStorage(OptionalStorage&& o) noexcept(std::is_nothrow_move_constructible_v) + : m_is_present(o.m_is_present), m_inactive() { if (m_is_present) { @@ -66,7 +76,8 @@ namespace vcpkg } } - OptionalStorage& operator=(const OptionalStorage& o) + OptionalStorage& operator=(const OptionalStorage& o) noexcept( + std::is_nothrow_copy_constructible_v&& std::is_nothrow_copy_assignable_v) { if (m_is_present && o.m_is_present) { @@ -84,7 +95,7 @@ namespace vcpkg return *this; } - OptionalStorage& operator=(OptionalStorage&& o) noexcept + OptionalStorage& operator=(OptionalStorage&& o) noexcept // enforces termination { if (m_is_present && o.m_is_present) { @@ -102,17 +113,17 @@ namespace vcpkg return *this; } - constexpr bool has_value() const { return m_is_present; } + constexpr bool has_value() const noexcept { return m_is_present; } - const T& value() const { return this->m_t; } - T& value() { return this->m_t; } + const T& value() const noexcept { return this->m_t; } + T& value() noexcept { return this->m_t; } - const T* get() const& { return m_is_present ? &m_t : nullptr; } - T* get() & { return m_is_present ? &m_t : nullptr; } + const T* get() const& noexcept { return m_is_present ? &m_t : nullptr; } + T* get() & noexcept { return m_is_present ? &m_t : nullptr; } const T* get() const&& = delete; T* get() && = delete; - void destroy() + void destroy() noexcept // enforces termination { m_is_present = false; m_t.~T(); @@ -120,7 +131,7 @@ namespace vcpkg } template - T& emplace(Args&&... args) + T& emplace(Args&&... args) noexcept(std::is_nothrow_constructible_v) { if (m_is_present) destroy(); new (&m_t) T(static_cast(args)...); @@ -141,14 +152,18 @@ namespace vcpkg struct OptionalStorage { constexpr OptionalStorage() noexcept : m_is_present(false), m_inactive() { } - constexpr OptionalStorage(T&& t) : m_is_present(true), m_t(std::move(t)) { } + constexpr OptionalStorage(T&& t) noexcept(std::is_nothrow_move_constructible_v) + : m_is_present(true), m_t(std::move(t)) + { + } - ~OptionalStorage() noexcept + ~OptionalStorage() { if (m_is_present) m_t.~T(); } - OptionalStorage(OptionalStorage&& o) noexcept : m_is_present(o.m_is_present), m_inactive() + OptionalStorage(OptionalStorage&& o) noexcept(std::is_nothrow_move_constructible_v) + : m_is_present(o.m_is_present), m_inactive() { if (m_is_present) { @@ -156,7 +171,7 @@ namespace vcpkg } } - OptionalStorage& operator=(OptionalStorage&& o) noexcept + OptionalStorage& operator=(OptionalStorage&& o) noexcept // enforces termination { if (m_is_present && o.m_is_present) { @@ -174,18 +189,18 @@ namespace vcpkg return *this; } - constexpr bool has_value() const { return m_is_present; } + constexpr bool has_value() const noexcept { return m_is_present; } - const T& value() const { return this->m_t; } - T& value() { return this->m_t; } + const T& value() const noexcept { return this->m_t; } + T& value() noexcept { return this->m_t; } - const T* get() const& { return m_is_present ? &m_t : nullptr; } - T* get() & { return m_is_present ? &m_t : nullptr; } + const T* get() const& noexcept { return m_is_present ? &m_t : nullptr; } + T* get() & noexcept { return m_is_present ? &m_t : nullptr; } const T* get() const&& = delete; T* get() && = delete; template - T& emplace(Args&&... args) + T& emplace(Args&&... args) noexcept(std::is_nothrow_constructible_v) { if (m_is_present) destroy(); new (&m_t) T(static_cast(args)...); @@ -193,7 +208,7 @@ namespace vcpkg return m_t; } - void destroy() + void destroy() noexcept { m_is_present = false; m_t.~T(); @@ -213,22 +228,22 @@ namespace vcpkg struct OptionalStorage { constexpr OptionalStorage() noexcept : m_t(nullptr) { } - constexpr OptionalStorage(T& t) : m_t(&t) { } - constexpr OptionalStorage(Optional& t) : m_t(t.get()) { } + constexpr OptionalStorage(T& t) noexcept : m_t(&t) { } + constexpr OptionalStorage(Optional& t) noexcept : m_t(t.get()) { } - constexpr bool has_value() const { return m_t != nullptr; } + constexpr bool has_value() const noexcept { return m_t != nullptr; } - T& value() const { return *this->m_t; } + T& value() const noexcept { return *this->m_t; } - T& emplace(T& t) + T& emplace(T& t) noexcept { m_t = &t; return *m_t; } - T* get() const { return m_t; } + T* get() const noexcept { return m_t; } - void destroy() { m_t = nullptr; } + void destroy() noexcept { m_t = nullptr; } private: T* m_t; @@ -238,25 +253,25 @@ namespace vcpkg struct OptionalStorage { constexpr OptionalStorage() noexcept : m_t(nullptr) { } - constexpr OptionalStorage(const T& t) : m_t(&t) { } - constexpr OptionalStorage(const Optional& t) : m_t(t.get()) { } - constexpr OptionalStorage(const Optional& t) : m_t(t.get()) { } + constexpr OptionalStorage(const T& t) noexcept : m_t(&t) { } + constexpr OptionalStorage(const Optional& t) noexcept : m_t(t.get()) { } + constexpr OptionalStorage(const Optional& t) noexcept : m_t(t.get()) { } constexpr OptionalStorage(Optional&& t) = delete; constexpr OptionalStorage(Optional&& t) = delete; - constexpr bool has_value() const { return m_t != nullptr; } + constexpr bool has_value() const noexcept { return m_t != nullptr; } - const T& value() const { return *this->m_t; } + const T& value() const noexcept { return *this->m_t; } - const T* get() const { return m_t; } + const T* get() const noexcept { return m_t; } - const T& emplace(const T& t) + const T& emplace(const T& t) noexcept { m_t = &t; return *m_t; } - void destroy() { m_t = nullptr; } + void destroy() noexcept { m_t = nullptr; } private: const T* m_t; @@ -270,13 +285,14 @@ namespace vcpkg constexpr Optional() noexcept { } // Constructors are intentionally implicit - constexpr Optional(NullOpt) { } + constexpr Optional(NullOpt) noexcept { } template, Optional> && std::is_constructible_v, U>, int> = 0> - constexpr Optional(U&& t) : details::OptionalStorage(static_cast(t)) + constexpr Optional(U&& t) noexcept(std::is_nothrow_constructible_v, U>) + : details::OptionalStorage(static_cast(t)) { } @@ -284,25 +300,25 @@ namespace vcpkg using details::OptionalStorage::has_value; using details::OptionalStorage::get; - T&& value_or_exit(const LineInfo& line_info) && + T&& value_or_exit(const LineInfo& line_info) && noexcept { Checks::check_exit(line_info, this->has_value(), "Value was null"); return std::move(this->value()); } - T& value_or_exit(const LineInfo& line_info) & + T& value_or_exit(const LineInfo& line_info) & noexcept { Checks::check_exit(line_info, this->has_value(), "Value was null"); return this->value(); } - const T& value_or_exit(const LineInfo& line_info) const& + const T& value_or_exit(const LineInfo& line_info) const& noexcept { Checks::check_exit(line_info, this->has_value(), "Value was null"); return this->value(); } - constexpr explicit operator bool() const { return this->has_value(); } + constexpr explicit operator bool() const noexcept { return this->has_value(); } template T value_or(U&& default_value) const& @@ -371,7 +387,7 @@ namespace vcpkg return nullopt; } - void clear() + void clear() noexcept { if (this->has_value()) { @@ -396,12 +412,6 @@ namespace vcpkg friend bool operator!=(const Optional& lhs, const Optional& rhs) noexcept { return !(lhs == rhs); } }; - template - Optional> make_optional(U&& u) - { - return Optional>(std::forward(u)); - } - // these cannot be hidden friends, unfortunately template auto operator==(const Optional& lhs, const Optional& rhs) -> decltype(*lhs.get() == *rhs.get()) diff --git a/src/vcpkg/base/system.process.cpp b/src/vcpkg/base/system.process.cpp index 521a2083ad..294c2e98fb 100644 --- a/src/vcpkg/base/system.process.cpp +++ b/src/vcpkg/base/system.process.cpp @@ -961,6 +961,7 @@ namespace RedirectedProcessInfo& operator=(const RedirectedProcessInfo&) = delete; ~RedirectedProcessInfo() = default; + VCPKG_MSVC_WARNING(suppress : 6262) // function uses 32k of stack int wait_and_stream_output(int32_t debug_id, const char* input, DWORD input_size, diff --git a/src/vcpkg/dependencies.cpp b/src/vcpkg/dependencies.cpp index 766536ef01..4130a417b5 100644 --- a/src/vcpkg/dependencies.cpp +++ b/src/vcpkg/dependencies.cpp @@ -263,7 +263,7 @@ namespace vcpkg } Checks::check_exit(VCPKG_LINE_INFO, !m_install_info.has_value()); - m_install_info = make_optional(ClusterInstallInfo{}); + m_install_info.emplace(); if (defaults_requested) { @@ -1045,12 +1045,12 @@ namespace vcpkg ActionPlan plan; - for (auto&& p_cluster : remove_toposort) + for (const auto* p_cluster : remove_toposort) { plan.remove_actions.emplace_back(p_cluster->m_spec, p_cluster->request_type); } - for (auto&& p_cluster : insert_toposort) + for (const auto* p_cluster : insert_toposort) { // Every cluster that has an install_info needs to be built // If a cluster only has an installed object and is marked as user requested we should still report it. @@ -1120,7 +1120,7 @@ namespace vcpkg m_graph->m_host_triplet, std::move(computed_edges), std::move(constraint_violations), - std::move(info_ptr->default_features)); + info_ptr->default_features); } else if (p_cluster->request_type == RequestType::USER_REQUESTED && p_cluster->m_installed.has_value()) { diff --git a/src/vcpkg/sourceparagraph.cpp b/src/vcpkg/sourceparagraph.cpp index c5f2772d75..abc00ef1a4 100644 --- a/src/vcpkg/sourceparagraph.cpp +++ b/src/vcpkg/sourceparagraph.cpp @@ -1271,14 +1271,14 @@ namespace vcpkg if (auto configuration = obj.get(VCPKG_CONFIGURATION)) { - if (!configuration->is_object()) + if (configuration->is_object()) { - r.add_generic_error(type_name(), - msg::format(msgJsonFieldNotObject, msg::json_field = VCPKG_CONFIGURATION)); + spgh.vcpkg_configuration.emplace(configuration->object(VCPKG_LINE_INFO)); } else { - spgh.vcpkg_configuration = make_optional(configuration->object(VCPKG_LINE_INFO)); + r.add_generic_error(type_name(), + msg::format(msgJsonFieldNotObject, msg::json_field = VCPKG_CONFIGURATION)); } }