From db02acbda20fb8e19143137945458929f15f0317 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 15 Oct 2021 14:08:24 +0200 Subject: PlayerManager: Replace debug assertions with warnings Parsing of input data might fail at runtime and is not a programming error. --- src/mixer/playermanager.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'src') diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index 4a2675fdde..25194123a5 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -36,13 +36,23 @@ const QRegularExpression kSamplerRegex(QStringLiteral("\\[Sampler\\d+\\]")); const QRegularExpression kPreviewDeckRegex(QStringLiteral("\\[PreviewDeck\\d+\\]")); bool extractIntFromRegex(const QRegularExpression& regex, const QString& group, int* number) { - QRegularExpressionMatch match = regex.match(group); + const QRegularExpressionMatch match = regex.match(group); + DEBUG_ASSERT(match.isValid()); if (!match.hasMatch()) { return false; } + // The regex is expected to contain a single capture group with the number + constexpr int capturedNumberIndex = 1; + DEBUG_ASSERT(match.lastCapturedIndex() <= capturedNumberIndex); + if (match.lastCapturedIndex() < capturedNumberIndex) { + qWarning() << "No number found in group" << group; + return false; + } if (number) { + const QString capturedNumber = match.captured(capturedNumberIndex); + DEBUG_ASSERT(!capturedNumber.isNull()); bool okay = false; - const int numberFromMatch = match.captured(1).toInt(&okay); + const int numberFromMatch = capturedNumber.toInt(&okay); VERIFY_OR_DEBUG_ASSERT(okay) { return false; } -- cgit v1.2.3 From 609f7f6d137c0ed7f73d4ea68c37547058c821f4 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 15 Oct 2021 14:53:39 +0200 Subject: PlayerManager: Fix regular expressions --- src/mixer/playermanager.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src') diff --git a/src/mixer/playermanager.cpp b/src/mixer/playermanager.cpp index 25194123a5..bf981f6664 100644 --- a/src/mixer/playermanager.cpp +++ b/src/mixer/playermanager.cpp @@ -31,9 +31,9 @@ const mixxx::Logger kLogger("PlayerManager"); // Utilize half of the available cores for adhoc analysis of tracks const int kNumberOfAnalyzerThreads = math_max(1, QThread::idealThreadCount() / 2); -const QRegularExpression kDeckRegex(QStringLiteral("\\[Channel\\d+\\]")); -const QRegularExpression kSamplerRegex(QStringLiteral("\\[Sampler\\d+\\]")); -const QRegularExpression kPreviewDeckRegex(QStringLiteral("\\[PreviewDeck\\d+\\]")); +const QRegularExpression kDeckRegex(QStringLiteral("^\\[Channel(\\d+)\\]$")); +const QRegularExpression kSamplerRegex(QStringLiteral("^\\[Sampler(\\d+)\\]$")); +const QRegularExpression kPreviewDeckRegex(QStringLiteral("^\\[PreviewDeck(\\d+)\\]$")); bool extractIntFromRegex(const QRegularExpression& regex, const QString& group, int* number) { const QRegularExpressionMatch match = regex.match(group); -- cgit v1.2.3 From 8bd73e6f88a9a9d5fa35b7f0b8ef75628f150f08 Mon Sep 17 00:00:00 2001 From: Uwe Klotz Date: Fri, 15 Oct 2021 14:12:10 +0200 Subject: FwdSqlQuery: Hide copy constructor --- src/library/trackset/crate/cratestorage.cpp | 3 +-- src/util/db/fwdsqlquery.h | 41 ++++++++++++++++++----------- src/util/db/fwdsqlqueryselectresult.cpp | 15 +++++------ src/util/db/sqlqueryfinisher.cpp | 19 ++++--------- src/util/db/sqlqueryfinisher.h | 34 ++++++++++++++---------- 5 files changed, 58 insertions(+), 54 deletions(-) (limited to 'src') diff --git a/src/library/trackset/crate/cratestorage.cpp b/src/library/trackset/crate/cratestorage.cpp index ee50e37fdf..a5a3da13d1 100644 --- a/src/library/trackset/crate/cratestorage.cpp +++ b/src/library/trackset/crate/cratestorage.cpp @@ -52,12 +52,11 @@ const QString kCrateSummaryViewQuery = CRATE_TABLE, CRATETABLE_ID); -class CrateQueryBinder { +class CrateQueryBinder final { public: explicit CrateQueryBinder(FwdSqlQuery& query) : m_query(query) { } - virtual ~CrateQueryBinder() = default; void bindId(const QString& placeholder, const Crate& crate) const { m_query.bindValue(placeholder, crate.getId()); diff --git a/src/util/db/fwdsqlquery.h b/src/util/db/fwdsqlquery.h index 5a08a04ba3..1ae1d864fd 100644 --- a/src/util/db/fwdsqlquery.h +++ b/src/util/db/fwdsqlquery.h @@ -14,22 +14,26 @@ class SqlQueryFinisher; class FwdSqlQuerySelectResult; -// A forward-only QSqlQuery that is prepared immediately -// during initialization. It offers a limited set of functions -// from QSqlQuery. -// -// Setting QSqlQuery to forward-only causes memory savings since -// QSqlCachedResult (what QtSQLite uses) won't allocate a giant -// in-memory table that we won't use at all when invoking only -// QSqlQuery::next() to iterate over the results. -// -// Prefer to use this derived class instead of QSqlQuery to avoid -// performance bottlenecks and for implicit logging failed query -// executions. -// -// Please note that forward-only queries don't provide information -// about the size of the result set! -class FwdSqlQuery : protected QSqlQuery { +/// A forward-only QSqlQuery that is prepared immediately +/// during initialization. It offers a limited set of functions +/// from QSqlQuery. +/// +/// Setting QSqlQuery to forward-only causes memory savings since +/// QSqlCachedResult (what QtSQLite uses) won't allocate a giant +/// in-memory table that we won't use at all when invoking only +/// QSqlQuery::next() to iterate over the results. +/// +/// Prefer to use this derived class instead of QSqlQuery to avoid +/// performance bottlenecks and for implicit logging failed query +/// executions. +/// +/// Please note that forward-only queries don't provide information +/// about the size of the result set! +/// +/// Since the destructor of the base class QSqlQuery is non-virtual +/// FwdSqlQuery must not contain any members with a non-trivial +/// destructor to prevent memory leaks! +class FwdSqlQuery final : protected QSqlQuery { friend class SqlQueryFinisher; friend class FwdSqlQuerySelectResult; @@ -40,6 +44,11 @@ class FwdSqlQuery : protected QSqlQuery { FwdSqlQuery( const QSqlDatabase& database, const QString& statement); + // The copy constructor of QSqlQuery is marked as deprecated in Qt6 + FwdSqlQuery(const FwdSqlQuery&) = delete; + FwdSqlQuery(FwdSqlQuery&&) = default; + + FwdSqlQuery& operator=(FwdSqlQuery&&) = default; bool isPrepared() const { return m_prepared; diff --git a/src/util/db/fwdsqlqueryselectresult.cpp b/src/util/db/fwdsqlqueryselectresult.cpp index 3543756825..0b0c52c270 100644 --- a/src/util/db/fwdsqlqueryselectresult.cpp +++ b/src/util/db/fwdsqlqueryselectresult.cpp @@ -1,8 +1,7 @@ #include "util/db/fwdsqlqueryselectresult.h" - FwdSqlQuerySelectResult::FwdSqlQuerySelectResult() - : m_queryFinisher(m_query) { + : m_queryFinisher(&m_query) { DEBUG_ASSERT(!m_query.isActive()); } @@ -12,11 +11,11 @@ FwdSqlQuerySelectResult::FwdSqlQuerySelectResult() // be less efficient than actually moving the object's contents, but // meets all requirements. FwdSqlQuerySelectResult::FwdSqlQuerySelectResult(FwdSqlQuery&& query) - : m_query(std::move(query)), - // Pass a reference to the member to m_queryFinisher, because - // the contents of the r-value reference parameter might have - // been moved! - m_queryFinisher(m_query) { + : m_query(std::move(query)), + // Pass a reference to the member to m_queryFinisher, because + // the contents of the r-value reference parameter might have + // been moved! + m_queryFinisher(&m_query) { DEBUG_ASSERT(m_query.isActive()); DEBUG_ASSERT(m_query.isSelect()); // Verify that the query has just been executed and that @@ -37,6 +36,6 @@ FwdSqlQuery FwdSqlQuerySelectResult::release() { FwdSqlQuery query(std::move(m_query)); // Ensure that the wrapped query is inaccessible after moving it! m_query = FwdSqlQuery(); - m_queryFinisher.release(); + DEBUG_ASSERT(!m_query.isActive()); return query; } diff --git a/src/util/db/sqlqueryfinisher.cpp b/src/util/db/sqlqueryfinisher.cpp index 2b9a40b5d1..7a46a36e09 100644 --- a/src/util/db/sqlqueryfinisher.cpp +++ b/src/util/db/sqlqueryfinisher.cpp @@ -1,19 +1,10 @@ #include "util/db/sqlqueryfinisher.h" -#include "util/assert.h" - - -bool SqlQueryFinisher::finish() { - if (m_query.isActive()) { - m_query.finish(); - release(); - return true; - } else { +bool SqlQueryFinisher::tryFinish() { + if (!m_pQuery || !m_pQuery->isActive()) { return false; } -} - -void SqlQueryFinisher::release() { - m_query = QSqlQuery(); - DEBUG_ASSERT(!m_query.isActive()); + m_pQuery->finish(); + m_pQuery = nullptr; + return true; } diff --git a/src/util/db/sqlqueryfinisher.h b/src/util/db/sqlqueryfinisher.h index a6fc06902c..8cc19087b8 100644 --- a/src/util/db/sqlqueryfinisher.h +++ b/src/util/db/sqlqueryfinisher.h @@ -2,32 +2,38 @@ #include +#include "util/db/fwdsqlquery.h" -// Ensures that reusable queries are properly finished when -// leaving the corresponding execution scope. This will free -// resources of prepared statements until the next execution. +/// Ensures that reusable queries are properly finished when +/// leaving the corresponding execution scope. This will free +/// resources of prepared statements until the next execution. class SqlQueryFinisher final { -public: - explicit SqlQueryFinisher(const QSqlQuery& query) - : m_query(query) { // implicitly shared (not copied) - } + public: + explicit SqlQueryFinisher(QSqlQuery* pQuery) + : m_pQuery(pQuery) { + } + explicit SqlQueryFinisher(FwdSqlQuery* pQuery) + : m_pQuery(pQuery) { + } SqlQueryFinisher(SqlQueryFinisher&& other) - : m_query(std::move(other.m_query)) { // implicitly shared (not moved) - other.release(); + : m_pQuery(other.m_pQuery) { + other.m_pQuery = nullptr; } ~SqlQueryFinisher() { - finish(); + tryFinish(); } - bool finish(); + bool tryFinish(); - void release(); + void release() { + m_pQuery = nullptr; + } -private: + private: // Disable copy construction and copy/move assignment SqlQueryFinisher(const SqlQueryFinisher&) = delete; SqlQueryFinisher& operator=(const SqlQueryFinisher&) = delete; SqlQueryFinisher& operator=(SqlQueryFinisher&&) = delete; - QSqlQuery m_query; // implicitly shared + QSqlQuery* m_pQuery; }; -- cgit v1.2.3