From 3ca600bd2d02a6950938789cef96229b15ec0cfa Mon Sep 17 00:00:00 2001 From: Andrei Golubev Date: Mon, 12 Oct 2020 11:30:51 +0200 Subject: [PATCH] Make QPromise::addResult() return boolean status of operation Changed QPromise::addResult() to return bool value. True is returned when result is added and false is returned when e.g. promise is in final state (canceled or finished) or when addResult() is called twice with the same index as argument (in which case new value is rejected) Updated QFutureInterface::reportFinished() that accepts optional result as argument to align with other result adding methods. This function is "internal" only (as of now), so no documentation update is needed Change-Id: I2d63069246e5e5c8cf04529c22bb296faaaae53d Reviewed-by: Lars Knoll Reviewed-by: Sona Kurazyan --- src/corelib/thread/qfutureinterface.h | 82 ++++++++++--------- src/corelib/thread/qpromise.h | 8 +- src/corelib/thread/qpromise.qdoc | 15 ++-- .../corelib/thread/qfuture/tst_qfuture.cpp | 80 +++++++++--------- .../corelib/thread/qpromise/tst_qpromise.cpp | 22 ++--- 5 files changed, 107 insertions(+), 100 deletions(-) diff --git a/src/corelib/thread/qfutureinterface.h b/src/corelib/thread/qfutureinterface.h index 9296c63f0b..8467047809 100644 --- a/src/corelib/thread/qfutureinterface.h +++ b/src/corelib/thread/qfutureinterface.h @@ -235,12 +235,12 @@ public: inline QFuture future(); // implemented in qfuture.h - inline void reportResult(const T *result, int index = -1); - inline void reportAndMoveResult(T &&result, int index = -1); - inline void reportResult(T &&result, int index = -1); - inline void reportResult(const T &result, int index = -1); - inline void reportResults(const QList &results, int beginIndex = -1, int count = -1); - inline void reportFinished(const T *result); + inline bool reportResult(const T *result, int index = -1); + inline bool reportAndMoveResult(T &&result, int index = -1); + inline bool reportResult(T &&result, int index = -1); + inline bool reportResult(const T &result, int index = -1); + inline bool reportResults(const QList &results, int beginIndex = -1, int count = -1); + inline bool reportFinished(const T *result); void reportFinished() { QFutureInterfaceBase::reportFinished(); @@ -259,32 +259,32 @@ public: }; template -inline void QFutureInterface::reportResult(const T *result, int index) +inline bool QFutureInterface::reportResult(const T *result, int index) { std::lock_guard locker{mutex()}; - if (this->queryState(Canceled) || this->queryState(Finished)) { - return; - } + if (this->queryState(Canceled) || this->queryState(Finished)) + return false; QtPrivate::ResultStoreBase &store = resultStoreBase(); + const int resultCountBefore = store.count(); + const int insertIndex = store.addResult(index, result); + if (insertIndex == -1) + return false; if (store.filterMode()) { - const int resultCountBefore = store.count(); - if (store.addResult(index, result) != -1) - this->reportResultsReady(resultCountBefore, store.count()); + this->reportResultsReady(resultCountBefore, store.count()); } else { - const int insertIndex = store.addResult(index, result); - if (insertIndex != -1) - this->reportResultsReady(insertIndex, insertIndex + 1); + this->reportResultsReady(insertIndex, insertIndex + 1); } + return true; } template -void QFutureInterface::reportAndMoveResult(T &&result, int index) +bool QFutureInterface::reportAndMoveResult(T &&result, int index) { std::lock_guard locker{mutex()}; if (queryState(Canceled) || queryState(Finished)) - return; + return false; QtPrivate::ResultStoreBase &store = resultStoreBase(); @@ -293,47 +293,50 @@ void QFutureInterface::reportAndMoveResult(T &&result, int index) // Let's make sure it's not in pending results. if (insertIndex != -1 && (!store.filterMode() || oldResultCount < store.count())) reportResultsReady(insertIndex, store.count()); + return insertIndex != -1; } template -void QFutureInterface::reportResult(T &&result, int index) +bool QFutureInterface::reportResult(T &&result, int index) { - reportAndMoveResult(std::move(result), index); + return reportAndMoveResult(std::move(result), index); } template -inline void QFutureInterface::reportResult(const T &result, int index) +inline bool QFutureInterface::reportResult(const T &result, int index) { - reportResult(&result, index); + return reportResult(&result, index); } template -inline void QFutureInterface::reportResults(const QList &_results, int beginIndex, int count) +inline bool QFutureInterface::reportResults(const QList &_results, int beginIndex, int count) { std::lock_guard locker{mutex()}; - if (this->queryState(Canceled) || this->queryState(Finished)) { - return; - } + if (this->queryState(Canceled) || this->queryState(Finished)) + return false; auto &store = resultStoreBase(); + const int resultCountBefore = store.count(); + const int insertIndex = store.addResults(beginIndex, &_results, count); + if (insertIndex == -1) + return false; if (store.filterMode()) { - const int resultCountBefore = store.count(); - if (store.addResults(beginIndex, &_results, count) != -1) - this->reportResultsReady(resultCountBefore, store.count()); + this->reportResultsReady(resultCountBefore, store.count()); } else { - const int insertIndex = store.addResults(beginIndex, &_results, count); - if (insertIndex != -1) - this->reportResultsReady(insertIndex, insertIndex + _results.count()); + this->reportResultsReady(insertIndex, insertIndex + _results.count()); } + return true; } template -inline void QFutureInterface::reportFinished(const T *result) +inline bool QFutureInterface::reportFinished(const T *result) { + bool resultReported = false; if (result) - reportResult(result); + resultReported = reportResult(result); reportFinished(); + return resultReported; } template @@ -427,9 +430,14 @@ public: inline QFuture future(); // implemented in qfuture.h - void reportResult(const void *, int) { } - void reportResults(const QList &, int) { } - void reportFinished(const void * = nullptr) + bool reportResult(const void *, int) { return false; } + bool reportResults(const QList &, int) { return false; } + bool reportFinished(const void *) + { + reportFinished(); + return false; + } + void reportFinished() { QFutureInterfaceBase::reportFinished(); QFutureInterfaceBase::runContinuation(); diff --git a/src/corelib/thread/qpromise.h b/src/corelib/thread/qpromise.h index 59c31de212..4225abd771 100644 --- a/src/corelib/thread/qpromise.h +++ b/src/corelib/thread/qpromise.h @@ -89,9 +89,9 @@ public: template>, typename = QtPrivate::EnableIfSameOrConvertible, std::decay_t>> - void addResult(U &&result, int index = -1) + bool addResult(U &&result, int index = -1) { - d.reportResult(std::forward(result), index); + return d.reportResult(std::forward(result), index); } #ifndef QT_NO_EXCEPTIONS void setException(const QException &e) { d.reportException(e); } @@ -118,8 +118,8 @@ public: } #if defined(Q_CLANG_QDOC) // documentation-only simplified signatures - void addResult(const T &result, int index = -1) { } - void addResult(T &&result, int index = -1) { } + bool addResult(const T &result, int index = -1) { } + bool addResult(T &&result, int index = -1) { } #endif private: mutable QFutureInterface d = QFutureInterface(); diff --git a/src/corelib/thread/qpromise.qdoc b/src/corelib/thread/qpromise.qdoc index 4b7b497b3f..279bc8766a 100644 --- a/src/corelib/thread/qpromise.qdoc +++ b/src/corelib/thread/qpromise.qdoc @@ -107,13 +107,17 @@ Returns a future associated with this promise. */ -/*! \fn template void QPromise::addResult(const T &result, int index = -1) +/*! \fn template bool QPromise::addResult(const T &result, int index = -1) + \fn template bool QPromise::addResult(T &&result, int index = -1) Adds \a result to the internal result collection at \a index position. If index is unspecified, \a result is added to the end of the collection. - \note addResult() rejects \a result if there's already another result in the - collection stored at the same index. + Returns \c true when \a result is added to the collection. + + Returns \c false when this promise is in cancelled or finished state or when + \a result is rejected. addResult() rejects \a result if there's already + another result in the collection stored at the same index. You can get a result at a specific index by calling QFuture::resultAt(). @@ -124,11 +128,6 @@ thinking if there are index gaps or not, use QFuture::results(). */ -/*! \fn template void QPromise::addResult(T &&result, int index = -1) - - \overload -*/ - /*! \fn template void QPromise::setException(const QException &e) Sets exception \a e to be the result of the computation. diff --git a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp index 99f2144528..2d78af8f94 100644 --- a/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp +++ b/tests/auto/corelib/thread/qfuture/tst_qfuture.cpp @@ -653,7 +653,7 @@ void tst_QFuture::futureInterface() { QFutureInterface i; i.reportStarted(); - i.reportResult(10); + QVERIFY(i.reportResult(10)); future = i.future(); i.reportFinished(); } @@ -674,7 +674,7 @@ void tst_QFuture::futureInterface() QCOMPARE(intFuture.isStarted(), true); QCOMPARE(intFuture.isFinished(), false); - result.reportFinished(&value); + QVERIFY(result.reportFinished(&value)); QCOMPARE(intFuture.isStarted(), true); QCOMPARE(intFuture.isFinished(), true); @@ -701,9 +701,9 @@ void tst_QFuture::futureInterface() { QFutureInterface i1; - i1.reportResult(1); + QVERIFY(i1.reportResult(1)); QFutureInterface i2; - i2.reportResult(2); + QVERIFY(i2.reportResult(2)); swap(i1, i2); // ADL must resolve this QCOMPARE(i1.resultReference(0), 2); QCOMPARE(i2.resultReference(0), 1); @@ -867,15 +867,15 @@ void tst_QFuture::multipleResults() int result; result = 1; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); QCOMPARE(f.resultAt(0), 1); result = 2; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); QCOMPARE(f.resultAt(1), 2); result = 3; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); result = 4; a.reportFinished(&result); @@ -916,16 +916,16 @@ void tst_QFuture::indexedResults() QChar result; result = 'B'; - Interface.reportResult(&result, 1); + QVERIFY(Interface.reportResult(&result, 1)); QCOMPARE(f.resultAt(1), result); result = 'A'; - Interface.reportResult(&result, 0); + QVERIFY(Interface.reportResult(&result, 0)); QCOMPARE(f.resultAt(0), result); result = 'C'; - Interface.reportResult(&result); // no index + QVERIFY(Interface.reportResult(&result)); // no index QCOMPARE(f.resultAt(2), result); Interface.reportFinished(); @@ -941,22 +941,22 @@ void tst_QFuture::indexedResults() int result; result = 0; - Interface.reportResult(&result, 0); + QVERIFY(Interface.reportResult(&result, 0)); QVERIFY(f.isResultReadyAt(0)); QCOMPARE(f.resultAt(0), 0); result = 3; - Interface.reportResult(&result, 3); + QVERIFY(Interface.reportResult(&result, 3)); QVERIFY(f.isResultReadyAt(3)); QCOMPARE(f.resultAt(3), 3); result = 2; - Interface.reportResult(&result, 2); + QVERIFY(Interface.reportResult(&result, 2)); QVERIFY(f.isResultReadyAt(2)); QCOMPARE(f.resultAt(2), 2); result = 4; - Interface.reportResult(&result); // no index + QVERIFY(Interface.reportResult(&result)); // no index QVERIFY(f.isResultReadyAt(4)); QCOMPARE(f.resultAt(4), 4); @@ -1013,7 +1013,7 @@ void tst_QFuture::resultsAfterFinished() QCOMPARE(f.resultCount(), 0); result = 1; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); QCOMPARE(f.resultAt(0), 1); a.reportFinished(); @@ -1021,7 +1021,7 @@ void tst_QFuture::resultsAfterFinished() QCOMPARE(f.resultAt(0), 1); QCOMPARE(f.resultCount(), 1); result = 2; - a.reportResult(&result); + QVERIFY(!a.reportResult(&result)); QCOMPARE(f.resultCount(), 1); } // cancel it @@ -1034,7 +1034,7 @@ void tst_QFuture::resultsAfterFinished() QCOMPARE(f.resultCount(), 0); result = 1; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); QCOMPARE(f.resultAt(0), 1); QCOMPARE(f.resultCount(), 1); @@ -1044,7 +1044,7 @@ void tst_QFuture::resultsAfterFinished() QCOMPARE(f.resultCount(), 1); result = 2; - a.reportResult(&result); + QVERIFY(!a.reportResult(&result)); a.reportFinished(); } } @@ -1057,9 +1057,9 @@ void tst_QFuture::resultsAsList() int result; result = 1; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); result = 2; - a.reportResult(&result); + QVERIFY(a.reportResult(&result)); a.reportFinished(); @@ -1500,7 +1500,7 @@ void tst_QFuture::voidConversions() QFuture intFuture(&iface); int value = 10; - iface.reportFinished(&value); + QVERIFY(iface.reportFinished(&value)); QFuture voidFuture(intFuture); voidFuture = intFuture; @@ -1515,7 +1515,7 @@ void tst_QFuture::voidConversions() iface.reportStarted(); QFuture > listFuture(&iface); - iface.reportResult(QList() << 1 << 2 << 3); + QVERIFY(iface.reportResult(QList() << 1 << 2 << 3)); voidFuture = listFuture; } QCOMPARE(voidFuture.resultCount(), 0); @@ -2853,8 +2853,8 @@ void tst_QFuture::takeResults() const int expectedCount = 10; for (int i = 0; i < expectedCount; ++i) { - moveIface.reportAndMoveResult(UniquePtr{new int(0b101010)}, i); - copyIface.reportAndMoveResult(std::vector{1,2,3,4,5}, i); + QVERIFY(moveIface.reportAndMoveResult(UniquePtr{new int(0b101010)}, i)); + QVERIFY(copyIface.reportAndMoveResult(std::vector{1,2,3,4,5}, i)); } moveIface.reportFinished(); @@ -2872,7 +2872,7 @@ void tst_QFuture::takeResult() { QFutureInterface iface; iface.reportStarted(); - iface.reportAndMoveResult(UniquePtr{new int(0b101010)}, 0); + QVERIFY(iface.reportAndMoveResult(UniquePtr{new int(0b101010)}, 0)); iface.reportFinished(); auto future = iface.future(); @@ -2956,9 +2956,9 @@ void tst_QFuture::resultsReadyAt() { int dummyResult = 0b101010; if (testMove) - iface.reportAndMoveResult(std::move(dummyResult), index); + QVERIFY(iface.reportAndMoveResult(std::move(dummyResult), index)); else - iface.reportResult(&dummyResult, index); + QVERIFY(iface.reportResult(&dummyResult, index)); }; const QSignalSpy readyCounter(&watcher, &QFutureWatcher::resultsReadyAt); @@ -3144,9 +3144,9 @@ void tst_QFuture::rejectResultOverwrite() // init if (initResults.size() == 1) - iface.reportResult(initResults[0]); + QVERIFY(iface.reportResult(initResults[0])); else - iface.reportResults(initResults); + QVERIFY(iface.reportResults(initResults)); QCOMPARE(f.resultCount(), initResults.size()); QCOMPARE(f.resultAt(0), initResults[0]); QCOMPARE(f.results(), initResults); @@ -3165,21 +3165,21 @@ void tst_QFuture::rejectResultOverwrite() { int result = -1; const auto originalCount = f.resultCount(); - iface.reportResult(result, 0); + QVERIFY(!iface.reportResult(result, 0)); QCOMPARE(f.resultCount(), originalCount); QCOMPARE(f.resultAt(0), initResults[0]); } // overwrite with rvalue { const auto originalCount = f.resultCount(); - iface.reportResult(-1, 0); + QVERIFY(!iface.reportResult(-1, 0)); QCOMPARE(f.resultCount(), originalCount); QCOMPARE(f.resultAt(0), initResults[0]); } // overwrite with array { const auto originalCount = f.resultCount(); - iface.reportResults(QList { -1, -2, -3 }, 0); + QVERIFY(!iface.reportResults(QList { -1, -2, -3 }, 0)); QCOMPARE(f.resultCount(), originalCount); QCOMPARE(f.resultAt(0), initResults[0]); } @@ -3187,7 +3187,7 @@ void tst_QFuture::rejectResultOverwrite() // special case: add result by different index, overlapping with the vector if (initResults.size() > 1) { const auto originalCount = f.resultCount(); - iface.reportResult(-1, 1); + QVERIFY(!iface.reportResult(-1, 1)); QCOMPARE(f.resultCount(), originalCount); QCOMPARE(f.resultAt(1), initResults[1]); } @@ -3221,9 +3221,9 @@ void tst_QFuture::rejectPendingResultOverwrite() // init if (initResults.size() == 1) - iface.reportResult(initResults[0], 1); + QVERIFY(iface.reportResult(initResults[0], 1)); else - iface.reportResults(initResults, 1); + QVERIFY(iface.reportResults(initResults, 1)); QCOMPARE(f.resultCount(), 0); // not visible yet if (!filterMode) { QCOMPARE(f.resultAt(1), initResults[0]); @@ -3244,7 +3244,7 @@ void tst_QFuture::rejectPendingResultOverwrite() { int result = -1; const auto originalCount = f.resultCount(); - iface.reportResult(result, 1); + QVERIFY(!iface.reportResult(result, 1)); QCOMPARE(f.resultCount(), originalCount); if (!filterMode) QCOMPARE(f.resultAt(1), initResults[0]); @@ -3252,7 +3252,7 @@ void tst_QFuture::rejectPendingResultOverwrite() // overwrite with rvalue { const auto originalCount = f.resultCount(); - iface.reportResult(-1, 1); + QVERIFY(!iface.reportResult(-1, 1)); QCOMPARE(f.resultCount(), originalCount); if (!filterMode) QCOMPARE(f.resultAt(1), initResults[0]); @@ -3260,7 +3260,7 @@ void tst_QFuture::rejectPendingResultOverwrite() // overwrite with array { const auto originalCount = f.resultCount(); - iface.reportResults(QList { -1, -2 }, 1); + QVERIFY(!iface.reportResults(QList { -1, -2 }, 1)); QCOMPARE(f.resultCount(), originalCount); if (!filterMode) QCOMPARE(f.resultAt(1), initResults[0]); @@ -3268,7 +3268,7 @@ void tst_QFuture::rejectPendingResultOverwrite() // special case: add result by different index, overlapping with the vector if (initResults.size() > 1) { const auto originalCount = f.resultCount(); - iface.reportResult(-1, 2); + QVERIFY(!iface.reportResult(-1, 2)); QCOMPARE(f.resultCount(), originalCount); if (!filterMode) QCOMPARE(f.resultAt(2), initResults[1]); @@ -3284,7 +3284,7 @@ void tst_QFuture::rejectPendingResultOverwrite() f.resume(); } - iface.reportResult(123, 0); // make results at 0 and 1 accessible + QVERIFY(iface.reportResult(123, 0)); // make results at 0 and 1 accessible QCOMPARE(f.resultCount(), initResults.size() + 1); QCOMPARE(f.resultAt(1), initResults[0]); initResults.prepend(123); diff --git a/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp b/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp index 62e3711d42..600c0fd150 100644 --- a/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp +++ b/tests/auto/corelib/thread/qpromise/tst_qpromise.cpp @@ -171,7 +171,7 @@ void tst_QPromise::addResult() // add as lvalue int resultAt0 = 456; { - promise.addResult(resultAt0); + QVERIFY(promise.addResult(resultAt0)); QCOMPARE(f.resultCount(), 1); QCOMPARE(f.result(), resultAt0); QCOMPARE(f.resultAt(0), resultAt0); @@ -179,14 +179,14 @@ void tst_QPromise::addResult() // add as rvalue { int result = 789; - promise.addResult(789); + QVERIFY(promise.addResult(789)); QCOMPARE(f.resultCount(), 2); QCOMPARE(f.resultAt(1), result); } // add at position { int result = 56238; - promise.addResult(result, 2); + QVERIFY(promise.addResult(result, 2)); QCOMPARE(f.resultCount(), 3); QCOMPARE(f.resultAt(2), result); } @@ -194,14 +194,14 @@ void tst_QPromise::addResult() { int result = -1; const auto originalCount = f.resultCount(); - promise.addResult(result, 0); + QVERIFY(!promise.addResult(result, 0)); QCOMPARE(f.resultCount(), originalCount); QCOMPARE(f.resultAt(0), resultAt0); // overwrite does not work } // add as rvalue at position and overwrite { const auto originalCount = f.resultCount(); - promise.addResult(-1, 0); + QVERIFY(!promise.addResult(-1, 0)); QCOMPARE(f.resultCount(), originalCount); QCOMPARE(f.resultAt(0), resultAt0); // overwrite does not work } @@ -223,9 +223,9 @@ void tst_QPromise::addResultOutOfOrder() { QPromise promise; auto f = promise.future(); - promise.addResult(456, 1); + QVERIFY(promise.addResult(456, 1)); QCOMPARE(f.resultCount(), 0); - promise.addResult(123, 0); + QVERIFY(promise.addResult(123, 0)); QList expected({123, 456}); RUN_TEST_FUNC(compareResults, f, expected); @@ -236,16 +236,16 @@ void tst_QPromise::addResultOutOfOrder() { QPromise promise; auto f = promise.future(); - promise.addResult(0, 0); - promise.addResult(1, 1); - promise.addResult(3, 3); // intentional gap here + QVERIFY(promise.addResult(0, 0)); + QVERIFY(promise.addResult(1, 1)); + QVERIFY(promise.addResult(3, 3)); // intentional gap here QList expectedWhenGapExists({0, 1}); RUN_TEST_FUNC(compareResults, f, expectedWhenGapExists); QCOMPARE(f.resultAt(3), 3); QList expectedWhenNoGap({0, 1, 2, 3}); - promise.addResult(2, 2); // fill a gap with a value + QVERIFY(promise.addResult(2, 2)); // fill a gap with a value RUN_TEST_FUNC(compareResults, f, expectedWhenNoGap); QCOMPARE(f.results(), expectedWhenNoGap); }