Skip to content

Commit

Permalink
fix: Watched applies the same noexcept spec to its dtor as Base
Browse files Browse the repository at this point in the history
  • Loading branch information
DNKpp committed Oct 7, 2024
1 parent 4efbabc commit 7da4b12
Show file tree
Hide file tree
Showing 3 changed files with 74 additions and 5 deletions.
24 changes: 22 additions & 2 deletions examples/Watcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,17 @@ TEST_CASE(
//! [watched lifetime-watcher violation]
constexpr auto action = []
{
struct not_nothrow_destructible
{
// explicitly make the destructor throwable, this isn't necessarily required in real tests
~not_nothrow_destructible() noexcept(false)
{
}
};

const mimicpp::Watched<
mimicpp::Mock<void()>,
mimicpp::LifetimeWatcher> watched{}; // let's create a watched mock
not_nothrow_destructible,
mimicpp::LifetimeWatcher> watched{}; // let's create a watched instance

// We purposely forget to define a destruction expectation.
// Due to this, a no-match will be reported during destruction.
Expand All @@ -39,6 +47,10 @@ TEST_CASE(
{
struct my_copyable // Mock isn't a copyable type, thus use a custom one for this example
{
// explicitly make the destructor throwable, this isn't necessarily required in real tests
~my_copyable() noexcept(false)
{
}
};

mimicpp::Watched<
Expand Down Expand Up @@ -77,6 +89,10 @@ TEST_CASE(
{
struct my_copyable // Mock isn't a copyable type, thus use a custom one for this example
{
// explicitly make the destructor throwable, this isn't necessarily required in real tests
~my_copyable() noexcept(false)
{
}
};

mimicpp::Watched<
Expand Down Expand Up @@ -111,6 +127,10 @@ TEST_CASE(
{
struct my_copyable // Mock isn't a copyable type, thus use a custom one for this example
{
// explicitly make the destructor throwable, this isn't necessarily required in real tests
~my_copyable() noexcept(false)
{
}
};

mimicpp::Watched<
Expand Down
37 changes: 35 additions & 2 deletions include/mimic++/LifetimeWatcher.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,9 @@ namespace mimicpp
public Watchers...
{
public:
~BasicWatched() = default;
~BasicWatched() noexcept(std::is_nothrow_destructible_v<Base>) // NOLINT(modernize-use-equals-default)
{
}

using Base::Base;

Expand All @@ -199,7 +201,7 @@ namespace mimicpp
public Watchers...
{
public:
~BasicWatched() noexcept(std::is_nothrow_destructible_v<Base>) override // NOLINT(modernize-use-equals-default)
~BasicWatched() noexcept(std::is_nothrow_destructible_v<Base>) override // NOLINT(modernize-use-equals-default)
{
}

Expand All @@ -216,6 +218,37 @@ namespace mimicpp
* \brief CRTP-type, inheriting first from ``Base`` and then all ``Watchers``, thus effectively couple them all together.
* \tparam Base The main type.
* \tparam Watchers All utilized watcher types.
* \details
* ## Destructors
*
* ``Watched`` automatically detects, whether ``Base`` has a virtual destructor and applies ``override`` if that's the case.
* It also forces the same ``noexcept``-ness for the destruct: If ``Base`` is nothrow destructible, ``Watched`` is it, too.
*
* This is important to note, as this has implications when a ``LifetimeWatcher`` is utilized.
* ``LifetimeWatcher`` may, during destruction, report violations to the currently active reporter. This reporter has to
* act accordingly, by either throwing an exception or terminating the program.
*
* As the destructor of the ``LifetimeWatcher`` will effectively be called from ``~Watched``, this will lead to a call to
* ``std::terminate``, if ``Base`` has a ``noexcept`` destructor (which is very likely, as it's a very strong default) and
* the reporter propagates the violation via an exception.
* \see https://en.cppreference.com/w/cpp/language/noexcept_spec
* \see https://en.cppreference.com/w/cpp/error/terminate
*
* There is no real way around that, beside explicitly ``~Watched`` as ``noexcept(false)``. Unfortunately, this would
* lead to inconsistencies with ``noexcept`` declared ``virtual`` destructors, because this requires all subclasses to match
* that specification.
* Besides that, there is an even stronger argument to strictly follow what ``Base`` offers:
* A ``Watched`` object should be as close to the original ``Base`` type as possible.
* If one wants to store a ``Watched<Base>`` inside e.g. ``std::vector`` and ``~Watched`` would have a different ``noexcept``
* specification than ``Base``, that would lead to behavior changes. This should never be the case,
* as mocks are expected to behave like an actual implementation-object.
*
* So, what does all of this mean?
*
* Actually, there are no implications to working tests. If they satisfy the expectations, no one will notice anything different.
* When it comes to a violation, which is detected by the destructor of a ``LifetimeWatcher``, the reporter will be notified and
* should print the no-match report to the console. After that, the program will than probably terminate (or halt, if a debugger
* is attached), but you should at least have an idea, which test is affected.
*/
template <typename Base, object_watcher... Watchers>
requires std::same_as<Base, std::remove_cvref_t<Base>>
Expand Down
18 changes: 17 additions & 1 deletion test/unit-tests/LifetimeWatcher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -312,12 +312,24 @@ TEST_CASE(
"Watched can wrap the actual type to be watched with the utilized watcher types.",
"[lifetime-watcher]")
{
STATIC_REQUIRE(std::is_nothrow_destructible_v<Watched<Mock<void(int)>, LifetimeWatcher>>);

SECTION("Detects violations.")
{
ScopedReporter reporter{};

struct not_nothrow_destructible
{
~not_nothrow_destructible() noexcept(false)
{
}
};

using WatcherT = Watched<not_nothrow_destructible, LifetimeWatcher>;
STATIC_REQUIRE(!std::is_nothrow_destructible_v<WatcherT>);

REQUIRE_THROWS_AS(
(Watched<Mock<void()>, LifetimeWatcher>{}),
WatcherT{},
NoMatchError);
}

Expand Down Expand Up @@ -385,6 +397,8 @@ TEST_CASE(
MIMICPP_MOCK_METHOD(foo, void, ());
};

STATIC_REQUIRE(std::is_nothrow_destructible_v<Watched<Derived, LifetimeWatcher>>);

auto watched = std::make_unique<Watched<Derived, LifetimeWatcher>>();

MIMICPP_SCOPED_EXPECTATION watched->expect_destruct();
Expand Down Expand Up @@ -414,6 +428,8 @@ TEST_CASE(
{
};

STATIC_REQUIRE(!std::is_nothrow_destructible_v<Watched<Derived, LifetimeWatcher>>);

ScopedReporter reporter{};

REQUIRE_THROWS_AS(
Expand Down

0 comments on commit 7da4b12

Please sign in to comment.