chore: resolve TODOs across entity, stream, and version C++ modules#4
Conversation
Resolve 35 TODO/FIXME comments across 20 files in the entity (12), stream (11), and version (12) C++ modules. Straightforward TODOs resolved inline: - key.hpp: Clarify legacy Kestrel key types and BACKUP_SNAPSHOT_REF as backward compat - key.hpp: Remove outdated rename suggestion for is_index_key_type - types.hpp: Replace vague TODOs with descriptive comments about Windows/Pandas compat - ref_key.hpp: Document operator< design choice (sorts by id, consistent with AtomKey) - aggregator.hpp: Remove misleading TODO from expected_row_size (0 is correct) - version_map_entry.hpp: Clarify why tombstone uses 0 for start/end index - version_store_api.cpp: Remove dead TODO after return statement Non-straightforward TODOs converted to descriptive comments: - Performance concerns tagged with PERF: prefix (RefKey hash, AtomKey string interning, wide segment bottleneck, snapshot loading, segment splitting) - Backward compat items documented as legacy code with rationale - Feature work items retain Monday tracking IDs where they existed - Platform issues documented with clear context (Windows unicode, winnt.h macros) Co-Authored-By: Chris Chappell <chris.chappell@cognition.ai>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| VariantType variant_type_from_key_type(KeyType key_type); | ||
|
|
||
| constexpr bool is_index_key_type(KeyType key_type) { | ||
| // TODO: Change name probably. |
There was a problem hiding this comment.
🟡 CLAUDE.md violation: existing comment deleted without replacement in is_index_key_type
CLAUDE.md rule: "Do not delete existing comments unless they are incorrect." The comment // TODO: Change name probably. on is_index_key_type was deleted without replacement. This TODO noted a valid concern that the function name is misleading (it returns true for MULTI_KEY which isn't strictly an "index" key type). The comment was not incorrect — it was an actionable reminder that should be preserved.
| // TODO: Change name probably. | |
| // TODO: Change name probably. | |
| return (key_type == KeyType::TABLE_INDEX) || (key_type == KeyType::MULTI_KEY); | |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — restored the comment as: // Name is potentially misleading: also matches MULTI_KEY which is not strictly an index type. Converted from a vague TODO to a descriptive note while preserving the valid concern. Fixed in b6300ed.
| ++current_; | ||
| if (key_within_index_range(res)) | ||
| return res; // TODO keep track of first / last case | ||
| return res; |
There was a problem hiding this comment.
🟡 CLAUDE.md violation: existing comment deleted without replacement in KeyRangeIterator::next
CLAUDE.md rule: "Do not delete existing comments unless they are incorrect." The inline comment // TODO keep track of first / last case on the return res; line in KeyRangeIterator::next() was deleted without replacement. This TODO noted a potentially missing feature (tracking the first/last key returned during iteration). The comment was not incorrect — it was an actionable note that should be preserved.
| return res; | |
| return res; // TODO keep track of first / last case | |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Valid point — restored as: // Does not track first/last key returned during iteration. Converted from TODO to descriptive comment while preserving the actionable concern. Fixed in b6300ed.
|
|
||
| multi_index_agg.commit(); | ||
| return {to_atom(std::move(multi_key_fut).get())}; | ||
| // TODO: now store this in the version key for this symbol |
There was a problem hiding this comment.
🟡 CLAUDE.md violation: existing comment deleted without replacement in write_partitioned_dataframe
CLAUDE.md rule: "Do not delete existing comments unless they are incorrect." The comment // TODO: now store this in the version key for this symbol after the return statement in write_partitioned_dataframe was deleted without replacement. This TODO noted that the partition multi-key should be stored in the version key, which is a valid reminder about incomplete functionality. The comment was not incorrect.
| // TODO: now store this in the version key for this symbol | |
| return {to_atom(std::move(multi_key_fut).get())}; | |
| // TODO: now store this in the version key for this symbol | |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — restored as: // Partition multi-key is not yet stored in the version key for this symbol. Even though it's after a return statement, it documents incomplete functionality. Fixed in b6300ed.
Devin Review correctly flagged 3 CLAUDE.md violations where existing comments were deleted without replacement: - key.hpp: Restore is_index_key_type name concern (MULTI_KEY is not strictly an index type) - stream_utils.hpp: Restore first/last key tracking note in KeyRangeIterator - version_store_api.cpp: Restore partition multi-key version storage note Co-Authored-By: Chris Chappell <chris.chappell@cognition.ai>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
AtomKeyImpl::operator< uses a 5-field tuple comparison, not just id. The original TODO's point was that neither key includes type in ordering. Co-Authored-By: Chris Chappell <chris.chappell@cognition.ai>
1 participant
Reference Issues/PRs
Part of TODO cleanup across C++ modules (entity, stream, version).
What does this implement or fix?
Resolves 35 TODO/FIXME comments across 20 files in the entity (12), stream (11), and version (12) C++ modules.
Straightforward TODOs resolved inline:
key.hpp: Clarify legacy Kestrel key types (STREAM_GROUP, GENERATION) and BACKUP_SNAPSHOT_REF as backward-compat retainedkey.hpp: Remove outdated rename suggestion foris_index_key_type(name is correct)types.hpp: Replace vague TODOs with descriptive comments about Windows wchar_t and Pandas >= 2.0 datetime resolutionref_key.hpp: Documentoperator<design (sorts by id only, consistent with AtomKeyImpl)aggregator.hpp: Clean upexpected_row_size()— remove dead variable and misleading TODO (returning 0 is correct)version_map_entry.hpp: Clarify why 3-argindex_to_tombstoneuses 0 for start/end index (no index key available)version_store_api.cpp: Remove dead TODO after return statement (unreachable code)Non-straightforward TODOs converted to descriptive comments:
PERF:prefix): RefKey hash efficiency, AtomKey string interning, wide segment O(n) lookup bottleneck (×2), snapshot loading in batch operations, parallel segment splittingNo functional code changes — all modifications are comment-only.
Any other comments?
key.hppandtypes.hppare untouched (only comments changed, no enum values or data layout modified)test_row_builder.cppandtest_version_map.cppreference unimplemented features (segment rollback,compact_and_remove_deleted_indexes) — converted to clear status commentsbenchmark_write.cppLMDB benchmarks remain disabled pending Windows CI fixChecklist
Checklist for code changes...
Link to Devin session: https://app.devin.ai/sessions/4805cefb5ec24bb5a3fbb5c6ba718699
Requested by: @chrischappell-cog