Fix reference parsing #96
Conversation
WalkthroughAdds a boundary-checked helper is_comma_or_not_end and replaces direct comma checks in OpenDDLParser::parseReference to prevent accessing beyond the input end. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Parser as OpenDDLParser::parseReference
participant Helper as is_comma_or_not_end
Caller->>Parser: parseReference(in, end)
loop Iterate references
Parser->>Helper: is_comma_or_not_end(in, end)?
alt not at end AND comma
Parser->>Parser: consume comma, advance
else end or not comma
Parser->>Parser: proceed without consuming
end
end
Parser-->>Caller: result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/OpenDDLParser.cpp (1)
1045-1046: Fix potential out-of-bounds read: dereferencing before end-check.The condition checks *in before verifying in != end. Reverse the order or reuse the new helper to make it safe and consistent.
Apply one of:
- } while (Grammar::CommaSeparator[0] == *in && in != end); + } while (in != end && Grammar::CommaSeparator[0] == *in);Or, using the helper for consistency:
- } while (Grammar::CommaSeparator[0] == *in && in != end); + } while (is_comma_safe(in, end));
🧹 Nitpick comments (1)
code/OpenDDLParser.cpp (1)
627-630: Good boundary guard; consider clearer name/signature and broader reuse.The helper prevents dereferencing past end. Two nits to improve clarity and consistency:
- Rename to reflect semantics (it returns true only when current char is a comma and we’re not at end). Example: is_comma_or_not_end → is_comma_and_not_end or simply is_comma_safe.
- Accept const char* to better convey that the function doesn’t mutate.
Apply this small refactor and update call sites:
-static bool is_comma_or_not_end(char *in, char *end) { - return in != end && Grammar::CommaSeparator[0] == *in; -} +static inline bool is_comma_safe(const char *in, const char *end) { + return in != end && Grammar::CommaSeparator[0] == *in; +}And later uses:
-while (is_comma_or_not_end(in, end)) { +while (is_comma_safe(in, end)) {- if (is_comma_or_not_end(in, end)) { + if (is_comma_safe(in, end)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
code/OpenDDLParser.cpp(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
code/OpenDDLParser.cpp (2)
test/OpenDDLParserTest.cpp (18)
in(87-87)in(427-427)in(453-453)in(475-475)in(505-505)in(561-561)in(616-616)end(88-88)end(322-322)end(336-336)end(347-347)end(359-359)end(370-370)end(396-396)end(519-519)end(551-551)end(595-595)end(616-616)test/OpenDDLParserUtilsTest.cpp (5)
end(38-38)end(282-282)end(293-293)end(318-318)end(328-328)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR (address)
🔇 Additional comments (1)
code/OpenDDLParser.cpp (1)
641-651: Refactor the comma-separated parsing loop inparseReferenceThe current loop in
code/OpenDDLParser.cpp(around lines 641–651 inOpenDDLParser::parseReference) only checks for a comma before and after callinggetNextSeparator, then breaks—so any comma-initiated names beyond the first never get parsed. Although your unit tests intest/OpenDDLParserTest.cppalready cover a two-element case ("$name1, %name2"), that mask may hide the loop’s flawed logic when more than two references are present.Please update the parsing to a straightforward “parse → while comma parse” pattern, for example:
- // after parsing the first name… - while (is_comma_or_not_end(in, end)) { - in = getNextSeparator(in, end); - if (is_comma_or_not_end(in, end)) { - in = parseName(in, end, &nextName); - if (nextName) { - names.push_back(nextName); - } - } else { - break; - } - } + // after parsing the first name, repeatedly consume ", <name>" + in = lookForNextToken(in, end); + while (in != end && *in == Grammar::CommaSeparator[0]) { + ++in; // skip the comma + in = lookForNextToken(in, end); // skip whitespace/comments + nextName = nullptr; + in = parseName(in, end, &nextName); + if (nextName) { + names.push_back(nextName); + } + in = lookForNextToken(in, end); // prepare for next comma + }Action items:
- Mandatory refactor in
OpenDDLParser.cppwithinparseReference(around lines 631–651).- Enhance tests in
test/OpenDDLParserTest.cppto include three or more comma-separated references (e.g."$a, %b, $c") and verify all are collected.This will both simplify the code and ensure full coverage of multi-reference inputs.
Summary by CodeRabbit