Skip to content

Commit

Permalink
Fix VS static analyzer warnings.
Browse files Browse the repository at this point in the history 
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 committed Oct 16, 2023
1 parent 7b53f45 commit 42a2949
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<T, Args...>)

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (ubuntu-20.04, linux-ci) 

expected primary-expression before ‘)’ token

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction

Check failure on line 203 in include/vcpkg/base/optional.h

View workflow job for this annotation

GitHub Actions / builds / build (macos-12, macos-ci) 

expected '(' for function-style cast or type construction
{
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 42a2949

Please sign in to comment.