From 968ebc61457aef475b56c65a3ad48e60bacc5dad Mon Sep 17 00:00:00 2001 From: Jens Geudens Date: Thu, 2 Apr 2026 21:00:58 +0200 Subject: [PATCH 1/2] Refactor expressionparser --- src/datahandling/expressionparser.cpp | 40 ++++++++++----------- src/datahandling/expressionparser.h | 15 ++++---- src/datahandling/graphdatahandler.cpp | 6 ++-- tests/datahandling/tst_expressionparser.cpp | 32 +++++++++++++---- tests/datahandling/tst_expressionparser.h | 6 +++- 5 files changed, 58 insertions(+), 41 deletions(-) diff --git a/src/datahandling/expressionparser.cpp b/src/datahandling/expressionparser.cpp index 89a67781..0191119b 100644 --- a/src/datahandling/expressionparser.cpp +++ b/src/datahandling/expressionparser.cpp @@ -6,30 +6,33 @@ const QString ExpressionParser::_cRegisterFunctionTemplate = "r(%1%2)"; -ExpressionParser::ExpressionParser(QStringList& expressions) +ExpressionParser::ExpressionParser(const QStringList& expressions) : _findRegRegex(ExpressionRegex::cMatchRegister) { - _findRegRegex.setPattern(ExpressionRegex::cMatchRegister); - _findRegRegex.optimize(); - parseExpressions(expressions); } -void ExpressionParser::dataPoints(QList& dataPointList) +/*! + * \brief Returns the deduplicated list of data points found during parsing. + */ +QList ExpressionParser::dataPoints() const { - dataPointList = _dataPoints; + return _dataPoints; } -void ExpressionParser::processedExpressions(QStringList& expressionList) +/*! + * \brief Returns the list of processed expressions with register references replaced. + */ +QStringList ExpressionParser::processedExpressions() const { - expressionList = _processedExpressions; + return _processedExpressions; } -void ExpressionParser::parseExpressions(QStringList& expressions) +void ExpressionParser::parseExpressions(const QStringList& expressions) { _processedExpressions.clear(); _dataPoints.clear(); - for (const QString& expression : std::as_const(expressions)) + for (const QString& expression : expressions) { _processedExpressions.append(processExpression(expression)); } @@ -42,8 +45,7 @@ QString ExpressionParser::processExpression(QString const& graphExpr) if (!i.hasNext() && resultExpr.contains("$")) { - auto msg = QString("Expression evaluation parsing failed (\"%1\")").arg(resultExpr); - qCWarning(scopeComm) << msg; + qCWarning(scopeComm) << QString("Expression evaluation parsing failed (\"%1\")").arg(resultExpr); } while (i.hasNext()) @@ -65,7 +67,7 @@ QString ExpressionParser::processExpression(QString const& graphExpr) return resultExpr; } -bool ExpressionParser::processRegisterExpression(QString regExpr, DataPoint& dataPoint) +bool ExpressionParser::processRegisterExpression(const QString& regExpr, DataPoint& dataPoint) { static const QRegularExpression regParseRegex(ExpressionRegex::cParseReg); QRegularExpressionMatch match = regParseRegex.match(regExpr); @@ -96,20 +98,16 @@ bool ExpressionParser::processRegisterExpression(QString regExpr, DataPoint& dat QString ExpressionParser::constructInternalRegisterFunction(DataPoint const& dataPoint, int size) { - quint32 idx; - if (_dataPoints.contains(dataPoint)) - { - idx = _dataPoints.indexOf(dataPoint); - } - else + qsizetype idx = _dataPoints.indexOf(dataPoint); + if (idx < 0) { _dataPoints.append(dataPoint); idx = _dataPoints.size() - 1; } /* Add dummy whitespaces to make sure positions in internal representations match visible expressions */ - QString regIdx = QString("%1").arg(idx); - const int spacesCount = size - 3 - regIdx.size(); /* ignore ${} and idx string length */ + QString regIdx = QString::number(idx); + const int spacesCount = qMax(0, size - 3 - regIdx.size()); /* ignore ${} and idx string length */ QString spaces = QString(" ").repeated(spacesCount); return QString(_cRegisterFunctionTemplate).arg(idx).arg(spaces); diff --git a/src/datahandling/expressionparser.h b/src/datahandling/expressionparser.h index 441c94de..3ca9aaeb 100644 --- a/src/datahandling/expressionparser.h +++ b/src/datahandling/expressionparser.h @@ -5,26 +5,25 @@ #include #include -class ExpressionParser : public QObject +class ExpressionParser { - Q_OBJECT public: - explicit ExpressionParser(QStringList& expressions); + explicit ExpressionParser(const QStringList& expressions); - void dataPoints(QList& dataPointList); - void processedExpressions(QStringList& expressionList); + QList dataPoints() const; + QStringList processedExpressions() const; private: - void parseExpressions(QStringList& expressions); + void parseExpressions(const QStringList& expressions); QString processExpression(QString const& expr); - bool processRegisterExpression(QString regExpr, DataPoint& dataPoint); + bool processRegisterExpression(const QString& regExpr, DataPoint& dataPoint); QString constructInternalRegisterFunction(DataPoint const& dataPoint, int size); QStringList _processedExpressions; QList _dataPoints; - QRegularExpression _findRegRegex; + const QRegularExpression _findRegRegex; static const QString _cRegisterFunctionTemplate; }; diff --git a/src/datahandling/graphdatahandler.cpp b/src/datahandling/graphdatahandler.cpp index 4e63ec6e..53c23f74 100644 --- a/src/datahandling/graphdatahandler.cpp +++ b/src/datahandling/graphdatahandler.cpp @@ -14,7 +14,6 @@ void GraphDataHandler::setupExpressions(GraphDataModel* pGraphDataModel, QList& registerList) { QStringList exprList; - QList regList; pGraphDataModel->activeGraphIndexList(&_activeIndexList); for (quint16 graphIdx : std::as_const(_activeIndexList)) @@ -23,12 +22,11 @@ void GraphDataHandler::setupExpressions(GraphDataModel* pGraphDataModel, QList regList = exprParser.dataPoints(); qCInfo(scopeComm) << "Active registers: " << DataPoint::dumpListToString(regList); - QStringList processedExpList; - exprParser.processedExpressions(processedExpList); + const QStringList processedExpList = exprParser.processedExpressions(); _valueParsers.clear(); diff --git a/tests/datahandling/tst_expressionparser.cpp b/tests/datahandling/tst_expressionparser.cpp index e68b492b..6d636564 100644 --- a/tests/datahandling/tst_expressionparser.cpp +++ b/tests/datahandling/tst_expressionparser.cpp @@ -236,16 +236,34 @@ void TestExpressionParser::manyRegisters() verifyParsing(input, expDataPoints, expExpressions); } -void TestExpressionParser::verifyParsing(QStringList exprList, - QList& expectedDataPoints, - QStringList& expectedExpression) +void TestExpressionParser::manyRegistersHighIndex() { - QList actualDataPoints; - QStringList actualExpressionList; + /* Verify qMax(0,...) guard: ${0} is 4 chars, but assigned index 10 (after 10 prior registers). + * Unguarded: spacesCount = 4-3-2 = -1. Guarded: clamped to 0, producing r(10) instead of garbage. */ + auto input = QStringList() << "${1}" << "${2}" << "${3}" << "${4}" << "${5}" + << "${6}" << "${7}" << "${8}" << "${9}" << "${10}" << "${0}"; + auto expDataPoints = QList() + << DataPoint("${1}", Device::cFirstDeviceId) << DataPoint("${2}", Device::cFirstDeviceId) + << DataPoint("${3}", Device::cFirstDeviceId) << DataPoint("${4}", Device::cFirstDeviceId) + << DataPoint("${5}", Device::cFirstDeviceId) << DataPoint("${6}", Device::cFirstDeviceId) + << DataPoint("${7}", Device::cFirstDeviceId) << DataPoint("${8}", Device::cFirstDeviceId) + << DataPoint("${9}", Device::cFirstDeviceId) << DataPoint("${10}", Device::cFirstDeviceId) + << DataPoint("${0}", Device::cFirstDeviceId); + /* ${10} at idx=9: size=5, spacesCount=5-3-1=1 → r(9 ) + * ${0} at idx=10: size=4, spacesCount=qMax(0,4-3-2)=0 → r(10) */ + auto expExpressions = QStringList() << "r(0)" << "r(1)" << "r(2)" << "r(3)" << "r(4)" + << "r(5)" << "r(6)" << "r(7)" << "r(8)" << "r(9 )" << "r(10)"; + verifyParsing(input, expDataPoints, expExpressions); +} + +void TestExpressionParser::verifyParsing(const QStringList& exprList, + const QList& expectedDataPoints, + const QStringList& expectedExpression) +{ ExpressionParser parser(exprList); - parser.dataPoints(actualDataPoints); - parser.processedExpressions(actualExpressionList); + const QList actualDataPoints = parser.dataPoints(); + const QStringList actualExpressionList = parser.processedExpressions(); QVERIFY(actualDataPoints == expectedDataPoints); QVERIFY(actualExpressionList == expectedExpression); diff --git a/tests/datahandling/tst_expressionparser.h b/tests/datahandling/tst_expressionparser.h index b6fd4004..08be8267 100644 --- a/tests/datahandling/tst_expressionparser.h +++ b/tests/datahandling/tst_expressionparser.h @@ -34,7 +34,11 @@ private slots: void constant(); void manyRegisters(); - void verifyParsing(QStringList exprList, QList& expectedDataPoints, QStringList& expectedExpression); + void manyRegistersHighIndex(); + + void verifyParsing(const QStringList& exprList, + const QList& expectedDataPoints, + const QStringList& expectedExpression); private: }; From c4e190482fe47170c07daf4c269d10906165d3f9 Mon Sep 17 00:00:00 2001 From: Jens Geudens Date: Thu, 2 Apr 2026 21:15:41 +0200 Subject: [PATCH 2/2] Add test --- tests/datahandling/tst_expressionparser.cpp | 9 +++++++++ tests/datahandling/tst_expressionparser.h | 1 + 2 files changed, 10 insertions(+) diff --git a/tests/datahandling/tst_expressionparser.cpp b/tests/datahandling/tst_expressionparser.cpp index 6d636564..24322179 100644 --- a/tests/datahandling/tst_expressionparser.cpp +++ b/tests/datahandling/tst_expressionparser.cpp @@ -134,6 +134,15 @@ void TestExpressionParser::multiRegistersDuplicate() verifyParsing(input, expDataPoints, expExpressions); } +void TestExpressionParser::singleExpressionDuplicate() +{ + auto input = QStringList() << "${45332} + ${45332}"; + auto expExpressions = QStringList() << "r(0 ) + r(0 )"; + auto expDataPoints = QList() << DataPoint("${45332}", Device::cFirstDeviceId); + + verifyParsing(input, expDataPoints, expExpressions); +} + void TestExpressionParser::failure() { auto input = QStringList() << "${}"; diff --git a/tests/datahandling/tst_expressionparser.h b/tests/datahandling/tst_expressionparser.h index 08be8267..bf4ef8b4 100644 --- a/tests/datahandling/tst_expressionparser.h +++ b/tests/datahandling/tst_expressionparser.h @@ -24,6 +24,7 @@ private slots: void singleRegisterConnType(); void multiRegisters(); void multiRegistersDuplicate(); + void singleExpressionDuplicate(); void failure(); void failureMulti(); void combinations();