Skip to content

Commit

Permalink
Fix VS static analyzer warnings. (#1235)
Browse files Browse the repository at this point in the history 
* 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<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.
  • Loading branch information
@BillyONeal
BillyONeal authored Oct 26, 2023
1 parent 8f4f3fa commit e0ec6cd
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 72 deletions.
22 changes: 13 additions & 9 deletions include/vcpkg/base/expected.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ namespace vcpkg
struct ExpectedHolder<T&>
{
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*;
Expand Down Expand Up @@ -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<Error>&& std::is_nothrow_copy_constructible_v<ExpectedHolder<T>>)
: value_is_error(other.value_is_error)
{
if (value_is_error)
{
Expand All @@ -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<Error>&& std::is_nothrow_move_constructible_v<ExpectedHolder<T>>)
: value_is_error(other.value_is_error)
{
if (value_is_error)
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -358,15 +362,15 @@ 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)
{
Checks::msg_exit_with_message(line_info, error());
}
}

void unreachable_if_error(const LineInfo& line_info) const
void unreachable_if_error(const LineInfo& line_info) const noexcept
{
if (value_is_error)
{
Expand Down
120 changes: 65 additions & 55 deletions include/vcpkg/base/optional.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>)
: m_is_present(true), m_t(t)
{
}
constexpr OptionalStorage(T&& t) noexcept(std::is_nothrow_move_constructible_v<T>)
: m_is_present(true), m_t(std::move(t))
{
}
template<class U, class = std::enable_if_t<!std::is_reference_v<U>>>
explicit OptionalStorage(Optional<U>&& t) : m_is_present(false), m_inactive()
explicit OptionalStorage(Optional<U>&& t) noexcept(std::is_nothrow_constructible_v<T, U>)
: m_is_present(false), m_inactive()
{
if (auto p = t.get())
{
Expand All @@ -39,7 +46,8 @@ namespace vcpkg
}
}
template<class U>
explicit OptionalStorage(const Optional<U>& t) : m_is_present(false), m_inactive()
explicit OptionalStorage(const Optional<U>& t) noexcept(std::is_nothrow_constructible_v<T, const U&>)
: m_is_present(false), m_inactive()
{
if (auto p = t.get())
{
Expand All @@ -48,25 +56,28 @@ 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<T>)
: 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<T>)
: m_is_present(o.m_is_present), m_inactive()
{
if (m_is_present)
{
new (&m_t) T(std::move(o.m_t));
}
}

OptionalStorage& operator=(const OptionalStorage& o)
OptionalStorage& operator=(const OptionalStorage& o) noexcept(
std::is_nothrow_copy_constructible_v<T>&& std::is_nothrow_copy_assignable_v<T>)
{
if (m_is_present && o.m_is_present)
{
Expand All @@ -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)
{
Expand All @@ -102,25 +113,25 @@ 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();
m_inactive = '\0';
}

template<class... Args>
T& emplace(Args&&... args)
T& emplace(Args&&... args) noexcept(std::is_nothrow_constructible_v<T, Args...>)
{
if (m_is_present) destroy();
new (&m_t) T(static_cast<Args&&>(args)...);
Expand All @@ -141,22 +152,26 @@ namespace vcpkg
struct OptionalStorage<T, false>
{
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<T>)
: 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<T>)
: m_is_present(o.m_is_present), m_inactive()
{
if (m_is_present)
{
new (&m_t) T(std::move(o.m_t));
}
}

OptionalStorage& operator=(OptionalStorage&& o) noexcept
OptionalStorage& operator=(OptionalStorage&& o) noexcept // enforces termination
{
if (m_is_present && o.m_is_present)
{
Expand All @@ -174,26 +189,26 @@ 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<class... Args>
T& emplace(Args&&... args)
T& emplace(Args&&... args) noexcept(std::is_nothrow_constructible_v<T, Args...>)
{
if (m_is_present) destroy();
new (&m_t) T(static_cast<Args&&>(args)...);
m_is_present = true;
return m_t;
}

void destroy()
void destroy() noexcept
{
m_is_present = false;
m_t.~T();
Expand All @@ -213,22 +228,22 @@ namespace vcpkg
struct OptionalStorage<T&, B>
{
constexpr OptionalStorage() noexcept : m_t(nullptr) { }
constexpr OptionalStorage(T& t) : m_t(&t) { }
constexpr OptionalStorage(Optional<T>& t) : m_t(t.get()) { }
constexpr OptionalStorage(T& t) noexcept : m_t(&t) { }
constexpr OptionalStorage(Optional<T>& 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;
Expand All @@ -238,25 +253,25 @@ namespace vcpkg
struct OptionalStorage<const T&, B>
{
constexpr OptionalStorage() noexcept : m_t(nullptr) { }
constexpr OptionalStorage(const T& t) : m_t(&t) { }
constexpr OptionalStorage(const Optional<T>& t) : m_t(t.get()) { }
constexpr OptionalStorage(const Optional<const T>& t) : m_t(t.get()) { }
constexpr OptionalStorage(const T& t) noexcept : m_t(&t) { }
constexpr OptionalStorage(const Optional<T>& t) noexcept : m_t(t.get()) { }
constexpr OptionalStorage(const Optional<const T>& t) noexcept : m_t(t.get()) { }
constexpr OptionalStorage(Optional<T>&& t) = delete;
constexpr OptionalStorage(Optional<const T>&& 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;
Expand All @@ -270,39 +285,40 @@ namespace vcpkg
constexpr Optional() noexcept { }

// Constructors are intentionally implicit
constexpr Optional(NullOpt) { }
constexpr Optional(NullOpt) noexcept { }

template<class U,
std::enable_if_t<!std::is_same_v<std::decay_t<U>, Optional> &&
std::is_constructible_v<details::OptionalStorage<T>, U>,
int> = 0>
constexpr Optional(U&& t) : details::OptionalStorage<T>(static_cast<U&&>(t))
constexpr Optional(U&& t) noexcept(std::is_nothrow_constructible_v<details::OptionalStorage<T>, U>)
: details::OptionalStorage<T>(static_cast<U&&>(t))
{
}

using details::OptionalStorage<T>::emplace;
using details::OptionalStorage<T>::has_value;
using details::OptionalStorage<T>::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<class U>
T value_or(U&& default_value) const&
Expand Down Expand Up @@ -371,7 +387,7 @@ namespace vcpkg
return nullopt;
}

void clear()
void clear() noexcept
{
if (this->has_value())
{
Expand All @@ -396,12 +412,6 @@ namespace vcpkg
friend bool operator!=(const Optional& lhs, const Optional& rhs) noexcept { return !(lhs == rhs); }
};

template<class U>
Optional<std::decay_t<U>> make_optional(U&& u)
{
return Optional<std::decay_t<U>>(std::forward<U>(u));
}

// these cannot be hidden friends, unfortunately
template<class T, class U>
auto operator==(const Optional<T>& lhs, const Optional<U>& rhs) -> decltype(*lhs.get() == *rhs.get())
Expand Down
1 change: 1 addition & 0 deletions src/vcpkg/base/system.process.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit e0ec6cd

Please sign in to comment.