ispringtech / FastSignals

Easy to use, fast and lightweight C++17 signals and slots library, drop-in replacement for the Boost.Signals2
MIT License
25 stars 10 forks source link

exception can be thrown inside packed_function noexcept move constructor and noexcept move assignment #13

Closed alexey-malov closed 5 years ago

alexey-malov commented 5 years ago

packed_function move constructor and move assignment operators are declared noexcept, however they invoke clone() method which, in turn, invokes Callable's copy constructor via clone() which may throw:

https://github.com/ispringteam/FastSignals/blob/5c05334245ba0391994788ce8a9cca32ac335fa9/libfastsignals/src/function_detail.cpp#L8-L20

https://github.com/ispringteam/FastSignals/blob/5c05334245ba0391994788ce8a9cca32ac335fa9/libfastsignals/src/function_detail.cpp#L27-L42

For instance, the following test will terminate because of exception within packed_function move constructor:

TEST_CASE("Move constructor must be noexcept", "[function]")
{
    struct ThrowsOnCopyIfNeeded
    {
        ThrowsOnCopyIfNeeded(bool& throwsOnCopy)
            : m_throwsOnCopy(&throwsOnCopy)
        {
        }
        ThrowsOnCopyIfNeeded(const ThrowsOnCopyIfNeeded& other)
            : m_throwsOnCopy(other.m_throwsOnCopy)
        {
            if (*other.m_throwsOnCopy)
            {
                throw std::runtime_error("Throw on request");
            }
        }
        ThrowsOnCopyIfNeeded& operator=(const ThrowsOnCopyIfNeeded& other)
        {
            m_throwsOnCopy = other.m_throwsOnCopy;
            return *this;
        }
        void operator()() {}

    private:
        bool* m_throwsOnCopy;
    };

    bool throwOnCopy = false;
    function<void()> fn{ThrowsOnCopyIfNeeded(throwOnCopy)};

    throwOnCopy = true;
    REQUIRE_NOTHROW((function<void()>(std::move(fn))));
}

In C++20 function's move constructor and move assignment operators will be noexcept. The possible implementation is to store data in small buffer IF the Callable is small enough to fit in buffer AND has nothrowing copy/move constructors

Warboss-rus commented 5 years ago

Fixed by introducing proper move support to function_proxy_impl

alexey-malov commented 5 years ago

I suggest adding a test checking that move constructor works as intended even if function body throws on copy/move

Warboss-rus commented 5 years ago

Added tests to check that function's move constructor uses callback's move constructor if it is noexcept and uses copy constructor otherwise