Adapt copyrights #91
Adapt copyrights #91
Conversation
WalkthroughThe PR updates license headers, modernizes several internal declarations (constexpr, using), marks an iterator final and defaulted its destructor, and tightens property parsing in OpenDDLParser::parseHeader. No public API signatures were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant P as Parser
participant L as Lexer
participant PH as PropertyChain
rect rgb(248,250,252)
Note over P: parseHeader() — read properties loop
P->>L: readToken()
alt token is property identifier
P->>PH: create property node
PH-->>P: property*
P->>L: peekNext()
alt next == "," or CloseProperty
P->>PH: link into chain (first/prev)
P->>L: continue or close
else invalid token
P-->>P: log error and abort
end
else end-of-input
P-->>P: break loop
else other tokens
P-->>P: existing handling
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
include/openddlparser/TPoolAllocator.h (2)
103-108: Bug: calling non-existent Pool(size_t) constructor — compile-time error.
new Pool(numItems);does not match any declared constructor; onlyPool()andPool(size_t, Pool*)exist. This should fail to compile.Apply this fix:
-inline TPoolAllocator<T>::TPoolAllocator(size_t numItems) : +inline TPoolAllocator<T>::TPoolAllocator(size_t numItems) : m_first(nullptr), m_current(nullptr), m_freeList(nullptr), m_capacity(0L) { - m_first = new Pool(numItems); + m_first = new Pool(numItems, nullptr); m_capacity += numItems; m_current = m_first; }
147-157: Bug: double-allocation leak in reserve().
new Pool(size, nullptr)already allocatesm_pool. Immediately allocatingm_current->m_pool = new T[size];leaks the first allocation and overwrites metadata.Apply this fix:
inline void TPoolAllocator<T>::reserve(size_t size) { clear(); - m_first = new Pool(size, nullptr); + m_first = new Pool(size, nullptr); m_current = m_first; - - m_current->m_pool = new T[size]; - m_current->m_poolsize = size; m_capacity = size; }code/OpenDDLParser.cpp (1)
296-326: Property list parsing: dropped linking and potential infinite loop on commas.Two problems in the while-loop after parsing a property:
- Current code links the parsed property only when the next token is NOT a comma, which causes only the last property before ')' to be kept. Properties followed by a comma are skipped.
- When the next token is a comma, it isn’t consumed, so the loop iterates again on the same comma and parseProperty() is called with the cursor at ',', leading to no progress and a potential infinite loop.
Fix by always linking the property when present, and explicitly consuming a comma before continuing.
Apply this diff:
- Property *prop{nullptr}, *prev{nullptr}; - while (in != end && *in != Grammar::ClosePropertyToken[0]) { - in = OpenDDLParser::parseProperty(in, end, &prop); - in = lookForNextToken(in, end); - if(in == end) { - break; - } - - if (*in != Grammar::CommaSeparator[0] && *in != Grammar::ClosePropertyToken[0]) { - logInvalidTokenError(std::string(in, end), Grammar::ClosePropertyToken, m_logCallback); - return nullptr; - } - - if (nullptr != prop && *in != Grammar::CommaSeparator[0]) { - if (nullptr == first) { - first = prop; - } - if (nullptr != prev) { - prev->m_next = prop; - } - prev = prop; - } - } - if(in != end) { - ++in; - } + Property *prop{nullptr}, *prev{nullptr}; + while (in != end && *in != Grammar::ClosePropertyToken[0]) { + in = OpenDDLParser::parseProperty(in, end, &prop); + // Always link parsed property (if any) + if (prop != nullptr) { + if (first == nullptr) { + first = prop; + } else { + prev->m_next = prop; + } + prev = prop; + } + // Next token handling + in = lookForNextToken(in, end); + if (in == end) { + break; + } + if (*in == Grammar::CommaSeparator[0]) { + ++in; // consume ',' + in = lookForNextToken(in, end); + continue; + } + if (*in != Grammar::ClosePropertyToken[0]) { + logInvalidTokenError(std::string(in, end), std::string(Grammar::ClosePropertyToken), m_logCallback); + return nullptr; + } + } + if (in != end && *in == Grammar::ClosePropertyToken[0]) { + ++in; // consume ')' + }code/OpenDDLExport.cpp (2)
43-51: Off-by-one: first child is skipped; single-child lists never iterate.Condition uses m_childs.size() > (m_idx + 1) and pre-increments m_idx, which:
- Skips index 0 on the first call.
- Fails entirely when size == 1.
Fix by returning the current element and then incrementing.
Apply this diff:
- bool getNext(DDLNode **node) { - if (m_childs.size() > (m_idx + 1)) { - m_idx++; - *node = m_childs[m_idx]; - return true; - } - - return false; - } + bool getNext(DDLNode **node) { + if (m_idx < m_childs.size()) { + *node = m_childs[m_idx++]; + return true; + } + return false; + }
134-159: writeNode() repeatedly overwrites ‘statement’ and uses ‘}’ where ‘{’ is expected.This drops previously built content and produces malformed output. Example issues:
- Assignments like
statement = "}"blow away the header/properties.- The node block appears to open with
}instead of{.- Only the last assigned fragment reaches writeToStream().
Minimal corrective rewrite below that appends instead of overwriting and fixes brace order.
- writeLineEnd(statement); - - statement = "}"; - DataArrayList *al(node->getDataArrayList()); - if (nullptr != al) { - writeValueType(al->m_dataList->m_type, al->m_numItems, statement); - writeValueArray(al, statement); - } - Value *v(node->getValue()); - if (nullptr != v) { - writeValueType(v->m_type, 1, statement); - statement = "{"; - writeLineEnd(statement); - writeValue(v, statement); - statement = "}"; - writeLineEnd(statement); - } - statement = "}"; - writeLineEnd(statement); - - writeToStream(statement); + writeLineEnd(statement); + // Open node block + statement += "{"; + writeLineEnd(statement); + + DataArrayList *al(node->getDataArrayList()); + Value *v(node->getValue()); + if (nullptr != al) { + // typed array payload: e.g., "int32[3] { 1, 2, 3 }" + writeValueType(al->m_dataList->m_type, al->m_numItems, statement); + statement += " "; + writeValueArray(al, statement); + writeLineEnd(statement); + } else if (nullptr != v) { + // typed single value payload + writeValueType(v->m_type, 1, statement); + statement += " {"; + writeLineEnd(statement); + writeValue(v, statement); + writeLineEnd(statement); + statement += "}"; + writeLineEnd(statement); + } + // Close node block + statement += "}"; + writeLineEnd(statement); + + writeToStream(statement);
🧹 Nitpick comments (12)
code/OpenDDLStream.cpp (1)
4-4: License header year bump to 2014–2025 — OK.No functional changes here; aligns with repo-wide update.
To avoid future year-churn across many files, consider adding an SPDX identifier alongside or instead of the full boilerplate:
// SPDX-License-Identifier: MITYou can keep the full text in one central location (e.g., LICENSE) and rely on SPDX in sources. A CI check (e.g., REUSE) can enforce this consistently.
include/openddlparser/OpenDDLStream.h (1)
4-4: Header license year updated to 2014–2025 — approved.Public declarations unchanged; no ABI impact.
If you move to SPDX (optional), mirror the identifier in headers as well to keep installed headers consistent with sources:
// SPDX-License-Identifier: MITinclude/openddlparser/OpenDDLCommon.h (1)
4-4: Address misspelled feature macro with backward-compatibility shimI located all occurrences of the misspelled macro and recommend adding a compatibility alias so we can switch to the correct name over time without breaking existing builds.
• include/openddlparser/OpenDDLCommon.h (line 35):
#ifdef OPENDDL_STATIC_LIBARY
• include/openddlparser/OpenDDLCommon.h (line 50):#endif // OPENDDL_STATIC_LIBARY
• CMakeLists.txt (line 115):target_compile_definitions(openddlparser PUBLIC OPENDDL_STATIC_LIBARY)Proposed follow-up (non-blocking) for a later PR:
--- a/include/openddlparser/OpenDDLCommon.h +++ b/include/openddlparser/OpenDDLCommon.h @@ // Back-compat shim; prefer OPENDDL_STATIC_LIBRARY #if defined(OPENDDL_STATIC_LIBARY) && !defined(OPENDDL_STATIC_LIBRARY) # define OPENDDL_STATIC_LIBRARY 1 #endif @@ -#ifdef OPENDDL_STATIC_LIBARY +#ifdef OPENDDL_STATIC_LIBRARY // …existing static‐library code… #endif // OPENDDL_STATIC_LIBRARYAnd in CMakeLists.txt:
--- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -115,7 +115,8 @@ target_include_directories(openddlparser PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/include) -target_compile_definitions(openddlparser PUBLIC OPENDDL_STATIC_LIBARY) +target_compile_definitions(openddlparser PUBLIC + OPENDDL_STATIC_LIBARY # old spelling + OPENDDL_STATIC_LIBRARY # new alias )This lets us keep building with the old macro while internally moving to
OPENDDL_STATIC_LIBRARY.include/openddlparser/TPoolAllocator.h (2)
195-200: Optional: report totals across all pools in dumpAllocations().Currently only reports the current pool’s index, which can be misleading with multiple pools.
Example improvement:
inline void TPoolAllocator<T>::dumpAllocations(std::string &allocs) { allocs.clear(); - allocs += "Number allocations = "; - allocs += std::to_string(m_current->m_currentIdx); + size_t total = 0; + for (Pool* p = m_first; p != nullptr; p = p->m_next) { + total += p->m_currentIdx; + } + allocs += "Number allocations (total across pools) = "; + allocs += std::to_string(total); allocs += "\n"; }
186-193: Nit: freeMem() ambiguity.The name suggests bytes, but it returns element count in the current pool only. Consider renaming or adjusting docs.
Suggested doc tweak:
-/// @brief Returns the unsigned int64 value. +/// @brief Returns the number of free elements in the current pool.Or rename to
freeElements()in a future, breaking-change PR.include/openddlparser/Value.h (4)
39-43: Fix typos and example in API docs.Minor doc issues: “many mode” → “many more”; “VylueType” → “ValueType”; example uses
it.getNextwithout().Apply:
-/// Values are used to store data types like boolean, integer, floats, double and many mode. To get -/// an overview please check the enum VylueType ( @see Value::ValueType ). +/// Values are used to store data types like boolean, integer, float, double and many more. To get +/// an overview please check the enum ValueType ( @see Value::ValueType ). @@ -/// Value v( it.getNext ); +/// Value* v = it.getNext();Also applies to: 52-57
91-94: Doc label mismatch: operator name.Comment says “The * operator” but the method is
operator->.Apply:
- /// @brief The * operator. + /// @brief The -> operator.
131-136: Param tag typo.Apply:
- /// @param value [in9 The value. + /// @param value [in] The value.
158-169: Copy-paste in return-value docs.Comments for
getInt32()andgetInt64()say “Returns the int16 value.”Apply:
- /// @brief Returns the int16 value. + /// @brief Returns the int32 value. @@ - /// @brief Returns the int16 value. + /// @brief Returns the int64 value.include/openddlparser/DDLNode.h (1)
59-59: Iterator alias modernization — consistent and fine.Consider also adding const_iterator if you iterate over const child lists elsewhere (optional).
include/openddlparser/OpenDDLParserUtils.h (1)
57-60: constexpr migration for CharTableSize and chartype_table is correct.
- Improves compile-time evaluation; “static constexpr” in a header keeps internal linkage per TU.
- Optional: consider std::array<unsigned char, CharTableSize> for stronger type-safety and size introspection.
code/OpenDDLParser.cpp (1)
342-353: Consistency nit: compare single chars uniformly.You use both dereference and [0] indexing for single-char tokens. For readability, prefer one style (e.g., Grammar::OpenBracketToken[0] / CloseBracketToken[0]) throughout parseStructure.
Apply this minimal diff for consistency:
- if (*in == *Grammar::OpenBracketToken) { + if (*in == Grammar::OpenBracketToken[0]) { ... - } while (in != end && *in != *Grammar::CloseBracketToken); + } while (in != end && *in != Grammar::CloseBracketToken[0]);
📜 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 (24)
code/DDLNode.cpp(1 hunks)code/OpenDDLCommon.cpp(1 hunks)code/OpenDDLExport.cpp(3 hunks)code/OpenDDLParser.cpp(8 hunks)code/OpenDDLStream.cpp(1 hunks)demo/main.cpp(1 hunks)include/openddlparser/DDLNode.h(2 hunks)include/openddlparser/OpenDDLCommon.h(1 hunks)include/openddlparser/OpenDDLExport.h(1 hunks)include/openddlparser/OpenDDLParser.h(1 hunks)include/openddlparser/OpenDDLParserUtils.h(2 hunks)include/openddlparser/OpenDDLStream.h(1 hunks)include/openddlparser/TPoolAllocator.h(3 hunks)include/openddlparser/Value.h(1 hunks)test/OpenDDLCommonTest.cpp(1 hunks)test/OpenDDLDefectsTest.cpp(1 hunks)test/OpenDDLExportTest.cpp(1 hunks)test/OpenDDLIntegrationTest.cpp(1 hunks)test/OpenDDLParserTest.cpp(1 hunks)test/OpenDDLParserUtilsTest.cpp(1 hunks)test/OpenDDLStreamTest.cpp(1 hunks)test/OssFuzzTest.cpp(1 hunks)test/UnitTestCommon.h(1 hunks)test/ValueTest.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
include/openddlparser/DDLNode.h (1)
code/DDLNode.cpp (2)
DDLNode(55-68)DDLNode(70-83)
⏰ 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 (26)
test/OpenDDLDefectsTest.cpp (1)
4-4: License header updated to 2014–2025 — LGTM.No code changes; tests remain unaffected.
test/OpenDDLCommonTest.cpp (2)
4-4: License year bumped to 2014-2025 — LGTM.Header text is consistent with MIT block, no code/test logic impacted.
4-4: Action Required: Update all copyright year ranges to “2014–2025”The quick scan revealed two source files still referencing “2014–2020” and the LICENSE lacks the updated range. Please update these to maintain consistency across the repo.
• test/DDLNodeTest.cpp (line 4)
- Copyright (c) 2014-2020 Kim Kulling + Copyright (c) 2014-2025 Kim Kulling• code/Value.cpp (line 4)
- Copyright (c) 2014-2020 Kim Kulling + Copyright (c) 2014-2025 Kim Kulling• LICENSE* files
- Ensure the main LICENSE (and any variant) mentions “2014–2025” where the copyright span appears.
Likely an incorrect or invalid review comment.
test/OssFuzzTest.cpp (1)
4-4: License year bumped to 2014-2025 — LGTM.No functional/test changes.
test/OpenDDLParserTest.cpp (1)
4-4: License year bumped to 2014-2025 — LGTM.No code/test logic altered.
code/OpenDDLCommon.cpp (1)
4-4: License year bumped to 2014-2025 — LGTM.Implementation remains unchanged; no behavioral impact.
test/ValueTest.cpp (1)
4-4: License year bumped to 2014-2025 — LGTM.No test behavior changes.
test/OpenDDLIntegrationTest.cpp (1)
4-4: License header year updated — OK.No test logic changes present.
test/UnitTestCommon.h (1)
4-4: License header year updated — OK.Header-only change; no functional impact.
include/openddlparser/TPoolAllocator.h (2)
65-67: Constructor formatting change is fine.Purely cosmetic consolidation of initializer list; no behavior change.
225-225: EOF newline / namespace macro closure — OK.Good to keep a terminating newline and explicit namespace end macro.
include/openddlparser/Value.h (1)
4-4: License header year updated — OK.include/openddlparser/DDLNode.h (2)
4-4: License header year bump looks good.No functional impact; aligns with PR objective.
56-56: Modernize typedef to using — good move.Clearer alias; no ABI impact.
code/DDLNode.cpp (1)
4-4: License header year update — OK.include/openddlparser/OpenDDLParserUtils.h (1)
4-4: License header year update — OK.test/OpenDDLParserUtilsTest.cpp (1)
4-4: License header year update — OK.code/OpenDDLParser.cpp (5)
4-4: License header year update — OK.
45-54: Use of constexpr for Grammar tokens is appropriate.Keeps literals in read-only storage and fits both char-comparisons and strncmp use sites.
112-114: Minor initialization cleanup — OK.Using brace init for type and explicit parent acquisition improves readability without changing semantics.
270-275: Init with brace syntax — OK.No behavior change; improves uniformity.
288-295: Name parsing: safe initialization — OK.Deleting name after use avoids leaks; no issues spotted.
include/openddlparser/OpenDDLExport.h (1)
4-4: License header updated to 2014-2025 — LGTM.No API or ABI changes in this header. Matches the PR objective.
test/OpenDDLExportTest.cpp (1)
4-4: License header updated to 2014-2025 — LGTM.Tests remain unaffected.
code/OpenDDLExport.cpp (2)
4-4: License header updated to 2014-2025 — LGTM.No behavioral change here.
32-41: Good modernization: make DDLNodeIterator final and default its dtor.Seals the type and removes an unnecessary out-of-class destructor definition. No runtime impact.
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)
296-327: Property-list parsing can infinite-loop and drops properties except the last.After parsing a property, the code:
- only links a property when the next token is not a comma, which means properties followed by commas are skipped (only the last property before ')' gets linked);
- does not consume the comma token, so
indoesn’t advance, leading to a tight infinite loop at the first comma.Fix by always linking when
propis non-null and explicitly consuming commas or the closing ')' token.Apply this diff to make the loop correct and progress safely:
- while (in != end && *in != Grammar::ClosePropertyToken[0]) { - in = OpenDDLParser::parseProperty(in, end, &prop); - in = lookForNextToken(in, end); - if(in == end) { - break; - } - - if (*in != Grammar::CommaSeparator[0] && *in != Grammar::ClosePropertyToken[0]) { - logInvalidTokenError(std::string(in, end), Grammar::ClosePropertyToken, m_logCallback); - return nullptr; - } - - if (nullptr != prop && *in != Grammar::CommaSeparator[0]) { - if (nullptr == first) { - first = prop; - } - if (nullptr != prev) { - prev->m_next = prop; - } - prev = prop; - } - } - if(in != end) { - ++in; - } + while (in != end) { + // parse one property + in = OpenDDLParser::parseProperty(in, end, &prop); + in = lookForNextToken(in, end); + if (in == end) { + // Unexpected EOF in property list + logInvalidTokenError(std::string(in, end), Grammar::ClosePropertyToken, m_logCallback); + return nullptr; + } + + // link parsed property (if any) + if (nullptr != prop) { + if (nullptr == first) { + first = prop; + } else if (nullptr != prev) { + prev->m_next = prop; + } + prev = prop; + } + + // expect ',' or ')' + if (*in == Grammar::CommaSeparator[0]) { + ++in; // consume comma and continue with next property + continue; + } + if (*in == Grammar::ClosePropertyToken[0]) { + ++in; // consume ')' + break; + } + + // anything else is invalid + logInvalidTokenError(std::string(in, end), Grammar::ClosePropertyToken, m_logCallback); + return nullptr; + }Also consider adding a quick unit test covering multiple properties, e.g.
(a=1, b=2, c="x"), to prevent regressions.
🧹 Nitpick comments (2)
code/OpenDDLParser.cpp (2)
45-53: constexpr token strings OK; use is correct but access style is inconsistent.You sometimes dereference the token pointer (
*Grammar::OpenBracketToken) and sometimes index (Grammar::OpenBracketToken[0]). Prefer one style (indexing) for readability and to avoid accidental misuse in future edits.Apply this tiny consistency fix within this file where feasible. Example for the changed loop condition (Line 354):
- } while (in != end && *in != *Grammar::CloseBracketToken); + } while (in != end && *in != Grammar::CloseBracketToken[0]);
343-354: Minor: condition reads clearer using[0]index like elsewhere.No semantic issue, just consistency/readability in the changed line.
- } while (in != end && *in != *Grammar::CloseBracketToken); + } while (in != end && *in != Grammar::CloseBracketToken[0]);
📜 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(8 hunks)
⏰ 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 (5)
code/OpenDDLParser.cpp (5)
4-4: Copyright range updated to 2025 — looks good.Change aligns with the PR’s stated objective.
112-114: createDDLNode modernization is fine.Brace-init for
typeand explicit parent retrieval read cleanly and don’t change semantics.
270-270: Local pointer init with brace-initialization is fine.No functional change; improves clarity.
288-295: Name parsing and assignment unchanged semantically — looks good.Memory ownership is handled as before (delete on success).
79-86: The above script will locate any instance of a stray backslash immediately following the opening brace of theif (callback)check. Once we see the results, we can confirm whether the backslash is still present (and thus needs removal) or already fixed.
|
Summary by CodeRabbit
Bug Fixes
Refactor
Chores