1berry/qtbase
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Qt Timers: disallow setting negative intervals

Browse Source 
As disccussed in the code review. Replace negative values with 1ms (not
0ms as that would spin up the event-loop too many times).

Move QTimer::start(std::chrono::milliseconds msec) API docs next to the
method definition so as to not forget changing it when the
implementation changes.

Drive-by, qWarning() and co. are marked cold so there is no need to use
Q_UNLIKELY in an if-condition leading to calling these methods.

[ChangeLog][QtCore][Important Behvaior Change] Timers with negative
intervals aren't allowed anymore, that is, if you try to start a timer
(or set the interval) with a negative value, that interval will be set
to 1ms. Previously Qt timers would let you set the interval to a
negative value, but behave in surprising ways (for example stop the
timer if it was running or not start it at all). This change affects
QTimer, QChronoTimer, QBasicTimer and QObject::startTimer().

Fixes: QTBUG-132359
Change-Id: I09309063665db051337f91160971993d9ce7911e
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
This commit is contained in:
Ahmad Samir 2024-12-19 14:10:31 +02:00
parent d4c312f68a
commit f1f610bc67
8 changed files with 133 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
src/corelib/kernel/qbasictimer.cpp
View File

@ -145,6 +145,8 @@ QT_BEGIN_NAMESPACE
The given \a object will receive timer events.
\include timers-common.qdocinc negative-intervals-not-allowed
//! [start-nanoseconds-note]
\note Starting from Qt 6.9 this method takes std::chrono::nanoseconds,
before that it took std::chrono::milliseconds. This change is
@ -170,6 +172,8 @@ QT_BEGIN_NAMESPACE
given \a timerType. See Qt::TimerType for information on the different
timer types.
\include timers-common.qdocinc negative-intervals-not-allowed
\a obj will receive timer events.
\include qbasictimer.cpp start-nanoseconds-note
@ -179,9 +183,10 @@ QT_BEGIN_NAMESPACE
void QBasicTimer::start(Duration duration, Qt::TimerType timerType, QObject *obj)
{
QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance();
if (Q_UNLIKELY(duration < 0ns)) {
qWarning("QBasicTimer::start: Timers cannot have negative timeouts");
return;
if (duration < 0ns) {
qWarning("QBasicTimer::start: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
duration = 1ms;
}
if (Q_UNLIKELY(!eventDispatcher)) {
qWarning("QBasicTimer::start: QBasicTimer can only be used with threads started with QThread");

8
src/corelib/kernel/qchronotimer.cpp
View File

@ -279,10 +279,18 @@ QBindable<bool> QChronoTimer::bindableSingleShot()
stop() and then start() the timer, and acquire a new id().
If the timer is not running, only the interval is changed.
\include timers-common.qdocinc negative-intervals-not-allowed
\sa singleShot
*/
void QChronoTimer::setInterval(std::chrono::nanoseconds nsec)
{
if (nsec < 0ns) {
qWarning("QChronoTimer::setInterval: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
nsec = 1ms;
}
auto *d = d_func();
d->intervalDuration.removeBindingUnlessInWrapper();
const bool intervalChanged = nsec != d->intervalDuration.valueBypassingBindings();

11
src/corelib/kernel/qobject.cpp
View File

@ -1821,6 +1821,8 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData
startTimer(std::chrono::milliseconds{interval}, timerType);
\endcode
\include timers-common.qdocinc negative-intervals-not-allowed
\sa timerEvent(), killTimer(), QChronoTimer, QBasicTimer
*/
@ -1843,6 +1845,8 @@ int QObject::startTimer(int interval, Qt::TimerType timerType)
then the timer event occurs once every time there are no more window
system events to process.
\include timers-common.qdocinc negative-intervals-not-allowed
The virtual timerEvent() function is called with the QTimerEvent
event parameter class when a timer event occurs. Reimplement this
function to get timer events.
@ -1887,9 +1891,10 @@ int QObject::startTimer(std::chrono::nanoseconds interval, Qt::TimerType timerTy
using namespace std::chrono_literals;
if (Q_UNLIKELY(interval < 0ns)) {
qWarning("QObject::startTimer: Timers cannot have negative intervals");
return 0;
if (interval < 0ns) {
qWarning("QObject::startTimer: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
interval = 1ms;
}
auto thisThreadData = d->threadData.loadRelaxed();

72
src/corelib/kernel/qtimer.cpp
View File

@ -233,6 +233,8 @@ void QTimer::start()
timer.start();
\endcode
\include timers-common.qdocinc negative-intervals-not-allowed
\note Keeping the event loop busy with a zero-timer is bound to
cause trouble and highly erratic behavior of the UI.
*/
@ -241,9 +243,40 @@ void QTimer::start(int msec)
start(msec * 1ms);
}
static std::chrono::milliseconds
checkInterval(const char *caller, std::chrono::milliseconds interval)
{
if (interval < 0ms) {
qWarning("%s: negative intervals aren't allowed; the interval will be set to 1ms.", caller);
interval = 1ms;
}
return interval;
}
/*!
\since 5.8
\overload
Starts or restarts the timer with a timeout of duration \a interval milliseconds.
\include qtimer.cpp stop-restart-timer
\include qtimer.cpp singleshot-activation
This is equivalent to:
\code
timer.setInterval(interval);
timer.start();
\endcode
\include timers-common.qdocinc negative-intervals-not-allowed
*/
void QTimer::start(std::chrono::milliseconds interval)
{
Q_D(QTimer);
interval = checkInterval("QTimer::start", interval);
// This could be narrowing as the interval is stored in an `int` QProperty,
// and the type can't be changed in Qt6.
const int msec = interval.count();
@ -370,6 +403,8 @@ void QTimer::singleShotImpl(std::chrono::nanoseconds ns, Qt::TimerType timerType
The \a receiver is the receiving object and the \a member is the
slot. The time interval is \a msec milliseconds.
\include timers-common.qdocinc negative-intervals-not-allowed
\sa start()
*/
@ -388,6 +423,8 @@ void QTimer::singleShotImpl(std::chrono::nanoseconds ns, Qt::TimerType timerType
time interval is \a msec milliseconds. The \a timerType affects the
accuracy of the timer.
\include timers-common.qdocinc negative-intervals-not-allowed
\sa start()
*/
@ -395,8 +432,9 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
const QObject *receiver, const char *member)
{
if (ns < 0ns) {
qWarning("QTimer::singleShot: Timers cannot have negative timeouts");
return;
qWarning("QTimer::singleShot: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
ns = 1ms;
}
if (receiver && member) {
if (ns == 0ns) {
@ -440,6 +478,8 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
The \a interval parameter can be an \c int (interpreted as a millisecond
count) or a \c std::chrono type that implicitly converts to nanoseconds.
\include timers-common.qdocinc negative-intervals-not-allowed
\note In Qt versions prior to 6.8, the chrono overloads took chrono::milliseconds,
not chrono::nanoseconds. The compiler will automatically convert for you,
but the conversion may overflow for extremely large milliseconds counts.
@ -462,6 +502,8 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
The \a receiver is the receiving object and the \a member is the slot. The
time interval is given in the duration object \a nsec.
\include timers-common.qdocinc negative-intervals-not-allowed
//! [qtimer-ns-overflow]
\note In Qt versions prior to 6.8, this function took chrono::milliseconds,
not chrono::nanoseconds. The compiler will automatically convert for you,
@ -487,6 +529,9 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
time interval is given in the duration object \a nsec. The \a timerType affects the
accuracy of the timer.
\include timers-common.qdocinc negative-intervals-not-allowed
\include qtimer.cpp qtimer-ns-overflow
\sa start()
@ -526,24 +571,6 @@ void QTimer::singleShot(std::chrono::nanoseconds ns, Qt::TimerType timerType,
\sa QObject::connect(), timeout()
*/
/*!
\fn void QTimer::start(std::chrono::milliseconds msec)
\since 5.8
\overload
Starts or restarts the timer with a timeout of duration \a msec milliseconds.
\include qtimer.cpp stop-restart-timer
\include qtimer.cpp singleshot-activation
This is equivalent to:
\code
timer.setInterval(msec);
timer.start();
\endcode
*/
/*!
\fn std::chrono::milliseconds QTimer::intervalAsDuration() const
\since 5.8
@ -604,6 +631,8 @@ QBindable<bool> QTimer::bindableSingleShot()
stop() and then start() the timer, and acquire a new id().
If the timer is not running, only the interval is changed.
\include timers-common.qdocinc negative-intervals-not-allowed
\sa singleShot
*/
void QTimer::setInterval(int msec)
@ -614,6 +643,9 @@ void QTimer::setInterval(int msec)
void QTimer::setInterval(std::chrono::milliseconds interval)
{
Q_D(QTimer);
interval = checkInterval("QTimer::setInterval", interval);
// This could be narrowing as the interval is stored in an `int` QProperty,
// and the type can't be changed in Qt6.
const int msec = interval.count();

8
src/corelib/kernel/timers-common.qdocinc
View File

@ -22,3 +22,11 @@
A disadvantage of using timerEvent() is that some high-level features,
such as single-shot timers and signals, aren't supported.
//! [q-chrono-timer-alternatives]
// \1 is the method argument's name e.g. `\a interval`
//! [negative-intervals-not-allowed]
Starting from Qt 6.10, setting a negative interval will result in a run-time warning
and the value being reset to 1ms. Before Qt 6.10 a Qt Timer would let you set a negative
interval but behave in surprising ways (for example stop the timer if it was running or
not start it at all).
//! [negative-intervals-not-allowed]

29
tests/auto/corelib/kernel/qchronotimer/tst_qchronotimer.cpp
View File

@ -1090,14 +1090,15 @@ void tst_QChronoTimer::bindToTimer()
auto ignoreMsg = [] {
QTest::ignoreMessage(QtWarningMsg,
"QObject::startTimer: Timers cannot have negative intervals");
"QChronoTimer::setInterval: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
};
ignoreMsg();
timer.setInterval(-100ms);
ignoreMsg();
timer.start();
QVERIFY(!active);
QVERIFY(active);
QCOMPARE(timer.interval(), 1ms);
timer.setInterval(100ms);
timer.start();
@ -1105,9 +1106,9 @@ void tst_QChronoTimer::bindToTimer()
ignoreMsg();
timer.setInterval(-100ms);
ignoreMsg();
timer.start();
QVERIFY(!active);
QVERIFY(active);
QCOMPARE(timer.interval(), 1ms);
}
void tst_QChronoTimer::bindTimer()
@ -1196,30 +1197,24 @@ void tst_QChronoTimer::negativeInterval()
auto ignoreMsg = [] {
QTest::ignoreMessage(QtWarningMsg,
"QObject::startTimer: Timers cannot have negative intervals");
"QChronoTimer::setInterval: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
};
ignoreMsg();
// Setting a negative interval does not change the active state.
timer.setInterval(-100ms);
ignoreMsg();
timer.start();
QVERIFY(!timer.isActive());
QVERIFY(timer.isActive());
QCOMPARE(timer.interval(), 1ms);
// Starting a timer that has a positive interval, the active state is changed
timer.setInterval(100ms);
timer.start();
QVERIFY(timer.isActive());
ignoreMsg();
// Setting a negative interval on an already running timer...
timer.setInterval(-100ms);
// ... the timer is stopped and the active state is changed
QVERIFY(!timer.isActive());
// Calling start on a timer that has a negative interval, does not change the active state
timer.start();
QVERIFY(!timer.isActive());
QVERIFY(timer.isActive());
QCOMPARE(timer.interval(), 1ms);
}
class OrderHelper : public QObject

12
tests/auto/corelib/kernel/qobject/tst_qobject.cpp
View File

@ -39,6 +39,7 @@
#include <math.h>
using namespace std::chrono_literals;
using namespace Qt::StringLiterals;
class tst_QObject : public QObject
@ -156,6 +157,7 @@ private slots:
void declarativeData();
void asyncCallbackHelper();
void disconnectQueuedConnection_pendingEventsAreDelivered();
void timerWithNegativeInterval();
};
struct QObjectCreatedOnShutdown
@ -8967,5 +8969,15 @@ void tst_QObject::disconnectQueuedConnection_pendingEventsAreDelivered()
QTRY_COMPARE(receiver.count_slot1, 1);
}
void tst_QObject::timerWithNegativeInterval()
{
QObject obj;
QTest::ignoreMessage(QtWarningMsg,
"QObject::startTimer: negative intervals aren't allowed; the "
"interval will be set to 1ms.");
int id = obj.startTimer(-100ms);
QCOMPARE_NE(Qt::TimerId{id}, Qt::TimerId::Invalid);
}
QTEST_MAIN(tst_QObject)
#include "tst_qobject.moc"

50
tests/auto/corelib/kernel/qtimer/tst_qtimer.cpp
View File

@ -1307,22 +1307,23 @@ void tst_QTimer::bindToTimer()
timer.stop();
QVERIFY(!active);
auto ignoreMsg = [] {
QTest::ignoreMessage(QtWarningMsg,
"QObject::startTimer: Timers cannot have negative intervals");
};
// also test that using negative interval updates the binding correctly
timer.start(100);
QVERIFY(active);
ignoreMsg();
QTest::ignoreMessage(QtWarningMsg,
"QTimer::setInterval: negative intervals aren't allowed; the interval "
"will be set to 1ms.");
timer.setInterval(-100);
QVERIFY(!active);
QVERIFY(active);
QCOMPARE(timer.intervalAsDuration(), 1ms);
timer.start(100);
QVERIFY(active);
ignoreMsg();
QTest::ignoreMessage(QtWarningMsg,
"QTimer::start: negative intervals aren't allowed; the interval "
"will be set to 1ms.");
timer.start(-100);
QVERIFY(!active);
QVERIFY(active);
QCOMPARE(timer.intervalAsDuration(), 1ms);
}
void tst_QTimer::bindTimer()
@ -1405,33 +1406,32 @@ void tst_QTimer::automatedBindingTests()
void tst_QTimer::negativeInterval()
{
auto ignoreMsg = [] {
QTest::ignoreMessage(QtWarningMsg,
"QObject::startTimer: Timers cannot have negative intervals");
};
QTimer timer;
// Starting with a negative interval does not change active state.
ignoreMsg();
QTest::ignoreMessage(QtWarningMsg,
"QTimer::start: negative intervals aren't allowed; the interval "
"will be set to 1ms.");
timer.start(-100ms);
QVERIFY(!timer.isActive());
QVERIFY(timer.isActive());
QCOMPARE(timer.intervalAsDuration(), 1ms);
// Updating the interval to a negative value stops the timer and changes
// the active state.
timer.start(100ms);
QVERIFY(timer.isActive());
ignoreMsg();
QTest::ignoreMessage(QtWarningMsg,
"QTimer::setInterval: negative intervals aren't allowed; the interval "
"will be set to 1ms.");
timer.setInterval(-100);
QVERIFY(!timer.isActive());
QVERIFY(timer.isActive());
QCOMPARE(timer.intervalAsDuration(), 1ms);
// Starting with a negative interval when already started leads to stop
// and inactive state.
timer.start(100);
QVERIFY(timer.isActive());
ignoreMsg();
QTest::ignoreMessage(QtWarningMsg,
"QTimer::start: negative intervals aren't allowed; the interval "
"will be set to 1ms.");
timer.start(-100ms);
QVERIFY(!timer.isActive());
QVERIFY(timer.isActive());
QCOMPARE(timer.intervalAsDuration(), 1ms);
}
class OrderHelper : public QObject