diff --git a/README.md b/README.md index 230e361..6ff4d2d 100644 --- a/README.md +++ b/README.md @@ -614,14 +614,14 @@ if (!result.HasErrors()) { | --- | --- | | `Error` / `FileOpenError` | Input cannot be opened | | `Error` / `FileReadError` | Input cannot be read completely | -| `Error` / `InvalidStatement` | Invalid frame/signal definition, duplicate, or payload violation | +| `Error` / `InvalidStatement` | Invalid frame/signal definition or comment, unresolved comment reference, duplicate, or payload violation | | `Error` / `InvalidValueDescription` | Invalid `VAL_` syntax, duplicate value, or unresolved reference | | `Error` / `InvalidMultiplexing` | Selector, branch range, or signal overlap validation failed | | `Warning` / `UnsupportedStatement` | Unregistered statement skipped; decoding or metadata may be incomplete | | `Error` / `UnsupportedStatement` | Extended multiplexing (`SG_MUL_VAL_`) rejected | Parsing stops at the first error and returns an empty database, together with -any preceding warnings and the error. Deferred `VAL_` and multiplexing errors +any preceding warnings and the error. Deferred comment, `VAL_`, and multiplexing errors retain their original source lines. Invalid signal markers use `InvalidStatement`. `HasErrors()` checks severity; warnings alone do not make a parse fail. @@ -637,8 +637,9 @@ indentation, even when they name unsupported syntax. Frame, signal, bus, and node definitions or a line with arguments end the list. Actual unsupported statements outside that list are reported. Node and bus metadata are not stored. Other comment forms (`CM_`, `CM_ BU_`, -`CM_ EV_`) are ignored by the comment handler; comments only attach to already -parsed frames/signals. Supported statements currently require one physical line. +`CM_ EV_`) are ignored by the comment handler. Frame and signal comments may span +physical lines and resolve after definitions are read; other supported statements +require one physical line. `ParseFile(path)` remains compatible: it returns the database, discards warnings, and throws `std::runtime_error` on the first error with the existing `path:line:` @@ -819,6 +820,19 @@ Stored in: - `FrameInfo::comment` - `SignalInfo::comment` +Comments accept spaces and tabs between fields, escaped quotes (`\"`) and +backslashes (`\\`), and text spanning multiple physical lines. Newlines, blank +lines, and indentation inside quoted text are preserved; CRLF becomes LF. +Comments may appear before their frame or signal definitions. References use the +same decimal CAN ID validation and normalization as `BO_`, including reserved-bit +and overflow checks and distinct standard/extended targets. Repeated comments +apply in statement order, so the last comment wins, including empty text. + +Malformed supported comments (including invalid escapes, missing quotes or +semicolon, and trailing text) and missing frame/signal references produce an +`InvalidStatement` error at the comment's starting line. Failed parses return an +empty database; `ParseFile` throws with the same source location. + --- ## CAN ID Normalization @@ -1149,7 +1163,8 @@ Additional behavior to account for: databases are also checked for ambiguous `VAL_` references during resolution - payload lengths are checked against storage bounds (`0..255`), rather than CAN or CAN FD protocol limits -- comment handling is limited to the supported forms and already parsed targets +- comment handling is limited to frame and signal comments; references resolve + after definitions are read Motorola extraction uses DBC sawtooth numbering: a start bit of 7 traverses `7..0, 15..8, 23..16`. Regression tests cover aligned and unaligned signals, diff --git a/changelog.md b/changelog.md index e88d055..191e9e9 100644 --- a/changelog.md +++ b/changelog.md @@ -8,6 +8,16 @@ existing Git tag; subsequent changes are listed as unreleased. ### 2026-10-14 +#### Fixed + +- Issue #18: frame/signal comments share frame-definition CAN ID validation and + resolve after definitions are read. Support tabs, escaped quotes/backslashes, + and multiline text while preserving whitespace and starting source locations. +- Report malformed comments and missing references as `InvalidStatement` errors; + keep statement-order overwrites, including empty comments. Added regression + coverage and updated parser/test documentation. Verified with `qmake6`, + `make -j4`, and `make check`: all six regression suites passed. + #### Changed - Validate duplicate frame keys before building the runtime lookup index, alongside diff --git a/core/dbc_parser.cpp b/core/dbc_parser.cpp index ea668d8..000ccb7 100644 --- a/core/dbc_parser.cpp +++ b/core/dbc_parser.cpp @@ -19,6 +19,36 @@ #include namespace { + // Read through the closing unescaped quote, preserving comment whitespace. + void ReadCommentLines(std::ifstream &input, std::string &statement, + std::size_t &lineNumber) { + bool inQuote = false; + bool escaped = false; + std::string part = statement; + for (;;) { + for (const char character: part) { + if (escaped) { + escaped = false; + } else if (inQuote && character == '\\') { + escaped = true; + } else if (character == '"') { + if (inQuote) + return; + inQuote = true; + } + } + if (!inQuote || !std::getline(input, part)) + return; + ++lineNumber; + if (!part.empty() && part.back() == '\r') + part.pop_back(); + statement += '\n'; + statement += part; + // A physical newline cannot escape the next line's first character. + escaped = false; + } + } + DbcDiagnosticCode StatementErrorCode(const std::string &keyword) { if (keyword == "SG_MUL_VAL_") return DbcDiagnosticCode::UnsupportedStatement; @@ -59,6 +89,10 @@ DbcParseResult DbcParser::ParseFileWithDiagnostics(const std::string &filePath) std::string line; while (std::getline(input, line)) { ++lineNumber; + if (!line.empty() && line.back() == '\r') + line.pop_back(); + const std::string rawLine = line; + const std::size_t statementLine = lineNumber; line = dbc_detail::TrimText(line); if (line.empty()) continue; @@ -74,19 +108,24 @@ DbcParseResult DbcParser::ParseFileWithDiagnostics(const std::string &filePath) continue; // NS_ lists syntax symbols; BO_, SG_, BS_ and BU_ end the list. inNamespace = keyword == "NS_" || keyword == "NS_:"; + if (keyword == "CM_") { + line = rawLine; + ReadCommentLines(input, line, lineNumber); + } + try { - context.lineNumber = lineNumber; + context.lineNumber = statementLine; if (!dispatcher.Dispatch(line, context)) { result.diagnostics.push_back({DbcDiagnosticSeverity::Warning, DbcDiagnosticCode::UnsupportedStatement, - filePath, lineNumber, + filePath, statementLine, "Unsupported statement '" + keyword + - "' is ignored; decoding or metadata may be incomplete."}); + "' is ignored; decoding or metadata may be incomplete."}); } } catch (const std::exception &error) { result.diagnostics.push_back({DbcDiagnosticSeverity::Error, StatementErrorCode(keyword), filePath, - lineNumber, error.what()}); + statementLine, error.what()}); return result; } } @@ -99,6 +138,7 @@ DbcParseResult DbcParser::ParseFileWithDiagnostics(const std::string &filePath) } try { + context.ResolveComments(filePath); context.ResolveValueDescriptions(filePath); context.ValidateMultiplexing(filePath); } catch (const DbcDiagnosticError &error) { diff --git a/core/dbc_parser.h b/core/dbc_parser.h index 2a3ef2f..4d4c62e 100644 --- a/core/dbc_parser.h +++ b/core/dbc_parser.h @@ -27,6 +27,8 @@ * - basic M / mN multiplexing * - VAL_ signal value descriptions * + * Frame/signal comments support multiline quoted text and deferred references. + * * Reports unsupported statements as warnings, except SG_MUL_VAL_ which is * an error. VERSION, NS_, BS_, BU_ and NS_ symbol declarations stay quiet. */ diff --git a/core/dbc_statement_handler.cpp b/core/dbc_statement_handler.cpp index a717950..7dcdd45 100644 --- a/core/dbc_statement_handler.cpp +++ b/core/dbc_statement_handler.cpp @@ -21,6 +21,15 @@ namespace { return value; } + std::uint32_t ParseCanId(const std::string &text) { + const std::uint32_t rawId = ParseDecimal( + text, std::numeric_limits::max(), "CAN ID"); + // Only bit 31 may accompany the actual 29-bit identifier. + if ((rawId & 0x60000000U) != 0U) + throw std::runtime_error("CAN ID exceeds the 29-bit identifier range."); + return rawId; + } + double ParsePhysicalValue(const std::string &text, const char *field) { static const std::regex number( R"(^[+-]?(?:[0-9]+(?:\.[0-9]*)?|\.[0-9]+)(?:[eE][+-]?[0-9]+)?$)"); @@ -193,11 +202,7 @@ FrameInfo DbcFrameHandler::ParseFrameLine(const std::string &line) { if (!(stream >> keyword >> idText) || keyword != "BO_") throw std::runtime_error("Missing CAN ID in frame definition."); - const std::uint32_t rawCanId = ParseDecimal( - idText, std::numeric_limits::max(), "CAN ID"); - // Only bit 31 may accompany the actual 29-bit identifier. - if ((rawCanId & 0x60000000U) != 0U) - throw std::runtime_error("CAN ID exceeds the 29-bit identifier range."); + const std::uint32_t rawCanId = ParseCanId(idText); std::string nameText; if (!std::getline(stream, nameText, ':') || stream.eof()) @@ -273,50 +278,6 @@ SignalInfo DbcSignalHandler::ParseSignalLine(const std::string &line) { return signal; } -void DbcCommentHandler::ParseCommentLine(const std::string &line, DbcDatabase &database) { - /* - * Examples: - * CM_ BO_ 256 "Frame comment"; - * CM_ SG_ 256 EngineSpeed "Signal comment"; - */ - - std::istringstream stream(line); - std::string token; - stream >> token; - - if (token != "CM_") - return; - - stream >> token; - if (token != "BO_" && token != "SG_") - return; - - std::uint32_t rawCanId = 0U; - std::uint32_t canId = 0U; - bool isExtended = false; - std::string signalName; - stream >> rawCanId; - if (token == "SG_") - stream >> signalName; - NormalizeCanId(rawCanId, canId, isExtended); - - const std::string::size_type quoteBegin = line.find('"'); - const std::string::size_type quoteEnd = line.rfind('"'); - if (quoteBegin == std::string::npos || quoteEnd == std::string::npos || quoteEnd <= quoteBegin) - return; - - FrameInfo *frame = FindFrameById(database, canId, isExtended); - if (frame == nullptr) - return; - const std::string comment = line.substr(quoteBegin + 1U, quoteEnd - quoteBegin - 1U); - if (token == "BO_") { - frame->comment = comment; - return; - } - SignalInfo *signal = FindSignalByName(*frame, signalName); - if (signal != nullptr) - signal->comment = comment; -} namespace { FrameInfo *FindFrameById(DbcDatabase &database, @@ -365,7 +326,67 @@ void DbcSignalHandler::Handle(const std::string &line, DbcParseContext &context) } void DbcCommentHandler::Handle(const std::string &line, DbcParseContext &context) const { - ParseCommentLine(line, context.database); + std::istringstream stream(line); + std::string keyword; + std::string target; + stream >> keyword >> target; + if (keyword != "CM_" || (target != "BO_" && target != "SG_")) + return; + + PendingComment pending; + pending.isSignal = target == "SG_"; + pending.lineNumber = context.lineNumber; + std::string idText; + if (!(stream >> idText)) + throw std::runtime_error("Missing CAN ID in CM_ statement."); + NormalizeCanId(ParseCanId(idText), pending.canId, pending.isExtended); + if (pending.isSignal && (!(stream >> pending.signalName) || + pending.signalName.find_first_of("\";") != std::string::npos)) + throw std::runtime_error("Missing or invalid signal name in CM_ statement."); + + stream >> std::ws; + if (stream.get() != '"') + throw std::runtime_error("Missing opening quote in CM_ statement."); + bool closed = false; + char character; + while (stream.get(character)) { + if (character == '"') { + closed = true; + break; + } + if (character == '\\') { + if (!stream.get(character) || (character != '"' && character != '\\')) + throw std::runtime_error("Invalid escape in CM_ statement."); + } + pending.comment += character; + } + if (!closed) + throw std::runtime_error("Unterminated CM_ comment."); + stream >> std::ws; + if (stream.get() != ';') + throw std::runtime_error("Missing semicolon in CM_ statement."); + stream >> std::ws; + if (!stream.eof()) + throw std::runtime_error("Unexpected trailing text in CM_ statement."); + context.pendingComments.push_back(std::move(pending)); +} + +void DbcParseContext::ResolveComments(const std::string &filePath) { + for (const PendingComment &pending: pendingComments) { + FrameInfo *frame = FindFrameById(database, pending.canId, pending.isExtended); + if (!frame) + throw DbcDiagnosticError(DbcDiagnosticCode::InvalidStatement, filePath, + pending.lineNumber, "Unresolved CM_ frame reference."); + if (pending.isSignal) { + SignalInfo *signal = FindSignalByName(*frame, pending.signalName); + if (!signal) + throw DbcDiagnosticError(DbcDiagnosticCode::InvalidStatement, filePath, + pending.lineNumber, "Unresolved CM_ signal reference: " + pending.signalName); + signal->comment = pending.comment; + } else { + frame->comment = pending.comment; + } + } } DbcStatementDispatcher::DbcStatementDispatcher() { diff --git a/core/dbc_statement_handler.h b/core/dbc_statement_handler.h index c18e3cf..e1e116e 100644 --- a/core/dbc_statement_handler.h +++ b/core/dbc_statement_handler.h @@ -17,6 +17,15 @@ struct PendingValueDescriptions { std::size_t lineNumber; }; +struct PendingComment { + std::uint32_t canId; + bool isExtended; + bool isSignal; + std::string signalName; + std::string comment; + std::size_t lineNumber; +}; + /** Carries deferred validation locations without parsing exception text. */ class DbcDiagnosticError : public std::runtime_error { public: @@ -38,6 +47,9 @@ struct DbcParseContext { std::vector> signalLines = {}; void ValidateMultiplexing(const std::string &filePath) const; + std::vector pendingComments = {}; + void ResolveComments(const std::string &filePath); + void ResolveValueDescriptions(const std::string &filePath); }; @@ -67,8 +79,6 @@ class DbcCommentHandler : public DbcStatementHandler { public: void Handle(const std::string &line, DbcParseContext &context) const override; -private: - static void ParseCommentLine(const std::string &line, DbcDatabase &database); }; class DbcValueDescriptionHandler : public DbcStatementHandler { diff --git a/tests/README.md b/tests/README.md index c863fad..35533bb 100644 --- a/tests/README.md +++ b/tests/README.md @@ -75,6 +75,11 @@ On Linux, reading a directory exercises the distinct `FileReadError` path. Parser tests cover standard and extended CAN IDs, whitespace, payload storage boundaries, missing fields, malformed and overflowing numbers, reserved ID bits, error paths and line numbers, and integration with signals and comments. +Comment regressions cover tabs, escaped quotes/backslashes, multiline text with +blank lines and indentation, CRLF, missing final newlines, comments preceding +frame/signal definitions, last-wins and empty overwrites, normalized ID boundaries, +standard/extended separation, malformed syntax/IDs, and missing targets. Errors +verify both structured diagnostics and the throwing API at the starting line. Duplicate definitions are rejected at their source lines, including equivalent extended ID encodings and repeated signal names within a frame. Standard and extended frames may share a numeric ID, and separate frames may reuse signal names. diff --git a/tests/dbc_coverage_test.cpp b/tests/dbc_coverage_test.cpp index 701c871..4a5191e 100644 --- a/tests/dbc_coverage_test.cpp +++ b/tests/dbc_coverage_test.cpp @@ -312,10 +312,7 @@ void CoverageTest::comments_data() { QTest::newRow("tabs") << QString::fromUtf8("") << QString::fromUtf8("CM_\tBO_ 1 \"Frame\";\nCM_\tSG_ 1 State \"Signal\";\n") << QString::fromUtf8("Frame") << QString::fromUtf8("Signal"); QTest::newRow("empty-overwrite") << QString::fromUtf8("") << QString::fromUtf8("CM_ BO_ 1 \"Old\";\nCM_ BO_ 1 \"\";\nCM_ SG_ 1 State \"Old\";\nCM_ SG_ 1 State \"\";\n") << QString::fromUtf8("") << QString::fromUtf8(""); QTest::newRow("last-wins") << QString::fromUtf8("") << QString::fromUtf8("CM_ BO_ 1 \"First\";\nCM_ BO_ 1 \"Last\";\nCM_ SG_ 1 State \"First\";\nCM_ SG_ 1 State \"Last\";\n") << QString::fromUtf8("Last") << QString::fromUtf8("Last"); - QTest::newRow("missing-targets") << QString::fromUtf8("") << QString::fromUtf8("CM_ BO_ 2 \"Unknown\";\nCM_ SG_ 2 State \"Unknown\";\nCM_ SG_ 1 Missing \"Unknown\";\n") << QString::fromUtf8("") << QString::fromUtf8(""); - // Current documented limitation: comments resolve only already parsed targets. - QTest::newRow("before-definitions") << QString::fromUtf8("CM_ BO_ 1 \"Early\";\nCM_ SG_ 1 State \"Early\";\n") << QString::fromUtf8("") << QString::fromUtf8("") << QString::fromUtf8(""); - QTest::newRow("missing-quotes") << QString::fromUtf8("") << QString::fromUtf8("CM_ BO_ 1 Missing;\nCM_ SG_ 1 State \"Unclosed;\n") << QString::fromUtf8("") << QString::fromUtf8(""); + QTest::newRow("before-definitions") << QString::fromUtf8("CM_ BO_ 1 \"Early\";\nCM_ SG_ 1 State \"Early\";\n") << QString::fromUtf8("") << QString::fromUtf8("Early") << QString::fromUtf8("Early"); } void CoverageTest::comments() { diff --git a/tests/dbc_parser_test.cpp b/tests/dbc_parser_test.cpp index 13e8305..875a0f0 100644 --- a/tests/dbc_parser_test.cpp +++ b/tests/dbc_parser_test.cpp @@ -75,6 +75,9 @@ private Q_SLOTS: void duplicateFrames(); void duplicateSignals(); void distinctDefinitions(); + void comments(); + void invalidComments_data(); + void invalidComments(); void integration(); void invalidSignals(); void validSignals(); @@ -489,6 +492,103 @@ void DbcParserTest::distinctDefinitions() { QCOMPARE(extended.definition->comment, std::string("Extended")); } +void DbcParserTest::comments() { + Fixture fixture; + fixture.Write( + "CM_\tBO_\t1\t\"First\";\r\n" + "CM_\tSG_\t1\tState\t\" Line one \r\n" + "\r\n" + "\tBO_ is text; \\\"quoted\\\" and \\\\slash\"\t;\r\n" + "CM_ BO_ 2147483649 \"Extended\";\r\n" + "CM_ SG_ 2147483649 State \"Extended signal\";\r\n" + "BO_ 1 Standard: 1 ECU\r\n" + "CM_ BO_ 1 \"Last\";\r\n" + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\r\n" + "BO_ 2147483649 Extended: 1 ECU\r\n" + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU"); + const DbcParseResult result = DbcParser().ParseFileWithDiagnostics(fixture.path.string()); + QVERIFY(!result.HasErrors()); + QVERIFY(result.diagnostics.empty()); + QCOMPARE(result.database.frames.size(), std::size_t(2)); + QCOMPARE(result.database.frames[0].comment, std::string("Last")); + QCOMPARE(result.database.frames[0].signals[0].comment, + std::string(" Line one \n\n\tBO_ is text; \"quoted\" and \\slash")); + QCOMPARE(result.database.frames[1].comment, std::string("Extended")); + QCOMPARE(result.database.frames[1].signals[0].comment, std::string("Extended signal")); + + const std::string ids[][2] = { + {"0", "0"}, + {"2047", "2047"}, + {"2048", "2147485696"}, + {"536870911", "2684354559"}, + {"2684354559", "536870911"}}; + for (const std::string(&id)[2]: ids) { + fixture.Write("CM_ BO_ " + id[0] + " \"Frame\";\nCM_ SG_ " + id[0] + + " State \"Signal\";\nBO_ " + id[1] + " Frame: 1 ECU\n" + "SG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n"); + const DbcDatabase database = DbcParser().ParseFile(fixture.path.string()); + QCOMPARE(database.frames[0].comment, std::string("Frame")); + QCOMPARE(database.frames[0].signals[0].comment, std::string("Signal")); + } + fixture.Write("CM_ BO_ 1 \"Line one\nLine two\";\n" + "BO_ 1 Frame: 0 ECU\nUNKNOWN_ value\nBO_ bad Frame: 0 ECU\n"); + const DbcParseResult following = DbcParser().ParseFileWithDiagnostics(fixture.path.string()); + QVERIFY(following.HasErrors()); + QCOMPARE(following.diagnostics.size(), std::size_t(2)); + QCOMPARE(following.diagnostics[0].lineNumber, std::size_t(4)); + QCOMPARE(following.diagnostics[1].lineNumber, std::size_t(5)); +} + +void DbcParserTest::invalidComments_data() { + QTest::addColumn("statement"); + const char *ids[] = {"-1", "+1", "1x", "0x1", "4294967296", "536870912", + "1073741824", "2684354560", "4294967295"}; + for (const char *id: ids) { + const std::string frameRow = std::string("frame-id-") + id; + const std::string signalRow = std::string("signal-id-") + id; + QTest::newRow(frameRow.c_str()) << QString::fromStdString("CM_ BO_ " + std::string(id) + " \"Text\";"); + QTest::newRow(signalRow.c_str()) << QString::fromStdString("CM_ SG_ " + std::string(id) + " State \"Text\";"); + } + QTest::newRow("missing-id") << QStringLiteral("CM_ BO_"); + QTest::newRow("missing-signal") << QStringLiteral("CM_ SG_ 1 \"Text\";"); + QTest::newRow("missing-quote") << QStringLiteral("CM_ BO_ 1 Text;"); + QTest::newRow("unclosed") << QStringLiteral("CM_ SG_ 1 State \"Text\n More"); + QTest::newRow("invalid-escape") << QStringLiteral("CM_ BO_ 1 \"bad\\q\";"); + QTest::newRow("dangling-escape") << QStringLiteral("CM_ BO_ 1 \"bad\\"); + QTest::newRow("missing-semicolon") << QStringLiteral("CM_ SG_ 1 State \"Text\""); + QTest::newRow("trailing-text") << QStringLiteral("CM_ BO_ 1 \"Text\"; garbage"); + QTest::newRow("extra-header") << QStringLiteral("CM_ BO_ 1 extra \"Text\";"); + QTest::newRow("extra-string") << QStringLiteral("CM_ BO_ 1 \"Text\" \"More\";"); + QTest::newRow("missing-frame") << QStringLiteral("CM_ BO_ 2 \"Text\";"); + QTest::newRow("missing-signal-frame") << QStringLiteral("CM_ SG_ 2 State \"Text\";"); + QTest::newRow("missing-signal-reference") << QStringLiteral("CM_ SG_ 1 Missing \"Text\";"); + QTest::newRow("different-frame-type") << QStringLiteral("CM_ BO_ 2147483649 \"Text\";"); + QTest::newRow("missing-multiline-reference") << QStringLiteral("CM_ SG_ 1 Missing \"Text\nMore\";"); +} + +void DbcParserTest::invalidComments() { + QFETCH(QString, statement); + Fixture fixture; + fixture.Write("BO_ 1 Frame: 1 ECU\nSG_ State : 0|8@1+ (1,0) [0|255] \"\" ECU\n\n" + + statement.toStdString()); + const DbcParseResult result = DbcParser().ParseFileWithDiagnostics(fixture.path.string()); + QVERIFY(result.HasErrors()); + QVERIFY(result.database.frames.empty()); + QCOMPARE(result.diagnostics.size(), std::size_t(1)); + const DbcDiagnostic &error = result.diagnostics[0]; + QCOMPARE(error.severity, DbcDiagnosticSeverity::Error); + QCOMPARE(error.code, DbcDiagnosticCode::InvalidStatement); + QCOMPARE(error.filePath, fixture.path.string()); + QCOMPARE(error.lineNumber, std::size_t(4)); + QVERIFY(!error.message.empty()); + try { + DbcParser().ParseFile(fixture.path.string()); + QFAIL("Malformed or unresolved comment must throw."); + } catch (const std::runtime_error &exception) { + QVERIFY(std::string(exception.what()).find(fixture.path.string() + ":4: ") == 0U); + } +} + void DbcParserTest::integration() { Fixture fixture; fixture.Write(