From 74c59f8a2350ed9ca640c42b641fd02d4e7c6dbd Mon Sep 17 00:00:00 2001 From: Nicolas Fella Date: Sun, 22 Aug 2021 19:21:45 +0200 Subject: [PATCH] QIcuTimeZonePrivate constructor: save iteration over all zone IDs ICU returns a "valid" representation of GMT when given an unrecognised ID, so QTZ's constructor has been checking the ID is available before passing it to the backend constructor. That availability check was done by generating the list of available IDs to see if the given ID was in it; this is very inefficient. Furthermore, the QTZ constructor was also checking availability, to work round the same issue in only this one backend, making the check redundant. So overide isTimeZoneIdAvailable() in the ICU backend, calling ucal_getCanonicalTimeZoneID(), which answers the question directly; and drop the duplicate check in the QTZ constructor. Expand a test to verify an invalid name is rejected. Fixes: QTBUG-121807 Pick-to: 6.6 6.5 Change-Id: I34f996b607b958d12607a94eb273bb1b406cca1a Reviewed-by: Thiago Macieira (cherry picked from commit 00d9a9a9b59650b8e297f91dcc600c377da5bceb) Reviewed-by: Volker Hilsheimer --- src/corelib/time/qtimezone.cpp | 17 ++++---------- src/corelib/time/qtimezoneprivate_icu.cpp | 23 +++++++++++++++++-- src/corelib/time/qtimezoneprivate_p.h | 1 + .../corelib/time/qtimezone/tst_qtimezone.cpp | 4 +++- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/corelib/time/qtimezone.cpp b/src/corelib/time/qtimezone.cpp index 6713bd0d48d..12c53ce024e 100644 --- a/src/corelib/time/qtimezone.cpp +++ b/src/corelib/time/qtimezone.cpp @@ -465,24 +465,15 @@ QTimeZone::Data &QTimeZone::Data::operator=(QTimeZonePrivate *dptr) noexcept QTimeZone::QTimeZone(const QByteArray &ianaId) { - // Try and see if it's a CLDR UTC offset ID - just as quick by creating as - // by looking up. + // Try and see if it's a recognized UTC offset ID - just as quick by + // creating as by looking up. d = new QUtcTimeZonePrivate(ianaId); - // If not a CLDR UTC offset ID then try creating it with the system backend. - // Relies on backend not creating valid TZ with invalid name. + // If not recognized, try creating it with the system backend. if (!d->isValid()) { if (ianaId.isEmpty()) d = newBackendTimeZone(); -#ifdef Q_OS_ANDROID - // on Android the isTimeZoneIdAvailable() implementation is vastly more - // expensive than just trying to create a timezone - else + else // Constructor MUST produce invalid for unsupported ID. d = newBackendTimeZone(ianaId); -#else - else if (global_tz->backend->isTimeZoneIdAvailable(ianaId)) - d = newBackendTimeZone(ianaId); -#endif - // else: No such ID, avoid creating a TZ cache entry for it. } // Can also handle UTC with arbitrary (valid) offset, but only do so as // fall-back, since either of the above may handle it more informatively. diff --git a/src/corelib/time/qtimezoneprivate_icu.cpp b/src/corelib/time/qtimezoneprivate_icu.cpp index c071e7d549d..d2f91d35e20 100644 --- a/src/corelib/time/qtimezoneprivate_icu.cpp +++ b/src/corelib/time/qtimezoneprivate_icu.cpp @@ -254,8 +254,8 @@ QIcuTimeZonePrivate::QIcuTimeZonePrivate() QIcuTimeZonePrivate::QIcuTimeZonePrivate(const QByteArray &ianaId) : m_ucal(nullptr) { - // Need to check validity here as ICu will create a GMT tz if name is invalid - if (availableTimeZoneIds().contains(ianaId)) + // ICU misleadingly maps invalid IDs to GMT. + if (isTimeZoneIdAvailable(ianaId)) init(ianaId); } @@ -442,6 +442,25 @@ QByteArray QIcuTimeZonePrivate::systemTimeZoneId() const return ucalDefaultTimeZoneId(); } +bool QIcuTimeZonePrivate::isTimeZoneIdAvailable(const QByteArray &ianaId) const +{ + const QString ianaStr = QString::fromUtf8(ianaId); + const UChar *const name = reinterpret_cast(ianaStr.constData()); + // We are not interested in the value, but we have to pass something. + // No known IANA zone name is (up to 2023) longer than 30 characters. + constexpr size_t size = 64; + UChar buffer[size]; + + // TODO: convert to ucal_getIanaTimeZoneID(), new draft in ICU 74, once we + // can rely on its availability, assuming it works the same once not draft. + UErrorCode status = U_ZERO_ERROR; + UBool isSys = false; + // Returns the length of the IANA zone name (but we don't care): + ucal_getCanonicalTimeZoneID(name, ianaStr.size(), buffer, size, &isSys, &status); + // We're only interested if the result is a "system" (i.e. IANA) ID: + return isSys; +} + QList QIcuTimeZonePrivate::availableTimeZoneIds() const { UErrorCode status = U_ZERO_ERROR; diff --git a/src/corelib/time/qtimezoneprivate_p.h b/src/corelib/time/qtimezoneprivate_p.h index f04157a654c..297e62c7781 100644 --- a/src/corelib/time/qtimezoneprivate_p.h +++ b/src/corelib/time/qtimezoneprivate_p.h @@ -233,6 +233,7 @@ public: QByteArray systemTimeZoneId() const override; + bool isTimeZoneIdAvailable(const QByteArray &ianaId) const override; QList availableTimeZoneIds() const override; QList availableTimeZoneIds(QLocale::Territory territory) const override; QList availableTimeZoneIds(int offsetFromUtc) const override; diff --git a/tests/auto/corelib/time/qtimezone/tst_qtimezone.cpp b/tests/auto/corelib/time/qtimezone/tst_qtimezone.cpp index 13a736a3281..09c80642a94 100644 --- a/tests/auto/corelib/time/qtimezone/tst_qtimezone.cpp +++ b/tests/auto/corelib/time/qtimezone/tst_qtimezone.cpp @@ -1281,7 +1281,9 @@ void tst_QTimeZone::icuTest() QIcuTimeZonePrivate tzpd; QVERIFY(tzpd.isValid()); - // Test invalid constructor + // Test invalid is not available: + QVERIFY(!tzpd.isTimeZoneIdAvailable("Gondwana/Erewhon")); + // and construction gives an invalid result: QIcuTimeZonePrivate tzpi("Gondwana/Erewhon"); QCOMPARE(tzpi.isValid(), false);