From 6deec9f51fe652a4f80c2cd8ea825357ac72a654 Mon Sep 17 00:00:00 2001 From: Simon Brunel Date: Sun, 22 Mar 2020 16:50:26 +0100 Subject: [PATCH] Fix support for auto args in constructor callbacks Broken since 78417b5, this bug creates an infinite recursion at runtime while trying to resolve the QPromise template constructor if the given callback is either an invalid function or a valid callback but using auto args (C++14). Related warning: C4717: recursive on all control paths, function will cause runtime stack overflow. --- src/qtpromise/qpromise.h | 2 +- src/qtpromise/qpromise.inl | 9 ++- src/qtpromise/qpromiseglobal.h | 6 +- tests/auto/qtpromise/CMakeLists.txt | 1 + tests/auto/qtpromise/cpp14/CMakeLists.txt | 10 +++ .../cpp14/tst_argsof_lambda_auto.cpp | 33 ++++++++ .../cpp14/tst_resolver_lambda_auto.cpp | 79 +++++++++++++++++++ 7 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 tests/auto/qtpromise/cpp14/CMakeLists.txt create mode 100644 tests/auto/qtpromise/cpp14/tst_argsof_lambda_auto.cpp create mode 100644 tests/auto/qtpromise/cpp14/tst_resolver_lambda_auto.cpp diff --git a/src/qtpromise/qpromise.h b/src/qtpromise/qpromise.h index 6976b5d..e5943a5 100644 --- a/src/qtpromise/qpromise.h +++ b/src/qtpromise/qpromise.h @@ -30,7 +30,7 @@ public: inline QPromiseBase(F resolver); template::count == 2, int>::type = 0> + typename std::enable_if::count != 1, int>::type = 0> inline QPromiseBase(F resolver); QPromiseBase(const QPromiseBase& other) : m_d{other.m_d} { } diff --git a/src/qtpromise/qpromise.inl b/src/qtpromise/qpromise.inl index e541219..d293fad 100644 --- a/src/qtpromise/qpromise.inl +++ b/src/qtpromise/qpromise.inl @@ -28,9 +28,16 @@ inline QPromiseBase::QPromiseBase(F callback) : m_d{new QtPromisePrivate::Pro } template -template::count == 2, int>::type> +template::count != 1, int>::type> inline QPromiseBase::QPromiseBase(F callback) : m_d{new QtPromisePrivate::PromiseData{}} { + // To prevent infinite recursion at runtime when resolving the QPromise template + // constructor, we don't explicitly check for ArgsOf::count == 2 so that this + // method is called for ALL callbacks other than the ones with a single typed + // argument. This includes valid callbacks such as with two args, variadic or + // auto args (c++14) but also invalid callbacks which are not functions or with + // 0 or more than 2 arguments, in which case this method MUST fail to compile. + QtPromisePrivate::PromiseResolver resolver{*this}; try { diff --git a/src/qtpromise/qpromiseglobal.h b/src/qtpromise/qpromiseglobal.h index 680eac6..44dd0c3 100644 --- a/src/qtpromise/qpromiseglobal.h +++ b/src/qtpromise/qpromiseglobal.h @@ -56,9 +56,11 @@ struct ArgsTraits<> static const size_t count = 0; }; -// Fallback implementation (type not supported). +// Fallback implementation, including types (T) which are not functions but +// also lambda with `auto` arguments, which are not covered but still valid +// callbacks (see the QPromiseBase template constructor). template -struct ArgsOf +struct ArgsOf : public ArgsTraits<> { }; // Partial specialization for null function. diff --git a/tests/auto/qtpromise/CMakeLists.txt b/tests/auto/qtpromise/CMakeLists.txt index fbcecf5..8d1ccd6 100644 --- a/tests/auto/qtpromise/CMakeLists.txt +++ b/tests/auto/qtpromise/CMakeLists.txt @@ -1,6 +1,7 @@ add_subdirectory(shared) add_subdirectory(benchmark) +add_subdirectory(cpp14) add_subdirectory(deprecations) add_subdirectory(exceptions) add_subdirectory(future) diff --git a/tests/auto/qtpromise/cpp14/CMakeLists.txt b/tests/auto/qtpromise/cpp14/CMakeLists.txt new file mode 100644 index 0000000..e7ed84f --- /dev/null +++ b/tests/auto/qtpromise/cpp14/CMakeLists.txt @@ -0,0 +1,10 @@ +# https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html +# https://gcc.gnu.org/projects/cxx-status.html#cxx14 +if ("cxx_generic_lambdas" IN_LIST CMAKE_CXX_COMPILE_FEATURES) + set(CMAKE_CXX_STANDARD 14) + qtpromise_add_tests(cpp14 + SOURCES + tst_argsof_lambda_auto.cpp + tst_resolver_lambda_auto.cpp + ) +endif() diff --git a/tests/auto/qtpromise/cpp14/tst_argsof_lambda_auto.cpp b/tests/auto/qtpromise/cpp14/tst_argsof_lambda_auto.cpp new file mode 100644 index 0000000..e6f0766 --- /dev/null +++ b/tests/auto/qtpromise/cpp14/tst_argsof_lambda_auto.cpp @@ -0,0 +1,33 @@ +/* + * Copyright (c) Simon Brunel, https://github.com/simonbrunel + * + * This source code is licensed under the MIT license found in + * the LICENSE file in the root directory of this source tree. + */ + +#include +#include + +using namespace QtPromisePrivate; + +class tst_cpp14_argsof_lambda_auto : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + void lambdaAutoArgs(); +}; + +QTEST_MAIN(tst_cpp14_argsof_lambda_auto) +#include "tst_argsof_lambda_auto.moc" + +void tst_cpp14_argsof_lambda_auto::lambdaAutoArgs() +{ + auto lOneArg = [](auto) {}; + auto lManyArgs = [](const auto&, auto, auto) {}; + auto lMutable = [](const auto&, auto) mutable {}; + + Q_STATIC_ASSERT((ArgsOf::count == 0)); + Q_STATIC_ASSERT((ArgsOf::count == 0)); + Q_STATIC_ASSERT((ArgsOf::count == 0)); +} diff --git a/tests/auto/qtpromise/cpp14/tst_resolver_lambda_auto.cpp b/tests/auto/qtpromise/cpp14/tst_resolver_lambda_auto.cpp new file mode 100644 index 0000000..3f4a7a7 --- /dev/null +++ b/tests/auto/qtpromise/cpp14/tst_resolver_lambda_auto.cpp @@ -0,0 +1,79 @@ +/* + * Copyright (c) Simon Brunel, https://github.com/simonbrunel + * + * This source code is licensed under the MIT license found in + * the LICENSE file in the root directory of this source tree. + */ + +#include "../shared/utils.h" + +#include +#include + +#include + +using namespace QtPromise; + +class tst_cpp14_resolver_lambda_auto : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + void resolverTwoAutoArgs(); + void resolverTwoAutoArgs_void(); +}; + +QTEST_MAIN(tst_cpp14_resolver_lambda_auto) +#include "tst_resolver_lambda_auto.moc" + +void tst_cpp14_resolver_lambda_auto::resolverTwoAutoArgs() +{ + QPromise p0{[](auto resolve, auto reject) { + Q_UNUSED(reject) + resolve(42); + }}; + QPromise p1{[](auto resolve, const auto& reject) { + Q_UNUSED(reject) + resolve(42); + }}; + QPromise p2{[](const auto& resolve, auto reject) { + Q_UNUSED(reject) + resolve(42); + }}; + QPromise p3{[](const auto& resolve, const auto& reject) { + Q_UNUSED(reject) + resolve(42); + }}; + + for (const auto& p : {p0, p1, p2, p3}) { + QCOMPARE(p.isFulfilled(), true); + QCOMPARE(waitForError(p, -1), -1); + QCOMPARE(waitForValue(p, -1), 42); + } +} + +void tst_cpp14_resolver_lambda_auto::resolverTwoAutoArgs_void() +{ + QPromise p0{[](auto resolve, auto reject) { + Q_UNUSED(reject) + resolve(); + }}; + QPromise p1{[](auto resolve, const auto& reject) { + Q_UNUSED(reject) + resolve(); + }}; + QPromise p2{[](const auto& resolve, auto reject) { + Q_UNUSED(reject) + resolve(); + }}; + QPromise p3{[](const auto& resolve, const auto& reject) { + Q_UNUSED(reject) + resolve(); + }}; + + for (const auto& p : {p0, p1, p2, p3}) { + QCOMPARE(p.isFulfilled(), true); + QCOMPARE(waitForError(p, -1), -1); + QCOMPARE(waitForValue(p, -1, 42), 42); + } +}