Repository navigation
form: widen the read path to 64-bit rows - #1022
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Storage index contract and propagation form/storage/istorage.hpp, form/storage/storage_reader.hpp, form/storage/storage_reader.cpp |
The storage reader API and index maps use std::uint64_t for row values. Index construction uses a 64-bit counter, and container reads pass token IDs without narrowing. |
Read-container implementations form/root_storage/root_rfield_read_container.*, form/root_storage/root_tbranch_read_container.*, form/storage/storage_read_container.* |
Read-container IDs and entry counts use std::uint64_t. The TBranch implementation uses std::cmp_greater_equal for its bounds check and casts the ID to Long64_t for LoadTree. |
Persistence and test consumers form/persistence/persistence_reader.cpp, test/form/form_multiple_outputs_check.cpp, test/form/navigation_check.hpp |
Persistence and navigation code use the 64-bit row types. The multiple-output check passes the index result without an explicit cast. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related PRs
- Framework-R-D/phlex#1007: Changes the
storage_reader::get_indexand index-map data flow, which this PR updates to use 64-bit row values.
Suggested reviewers: aolivier23, knoepfel
Merge Risk: 
3fca8
-
The read path supports rows beyond the previous 32-bit limit. No merge-blocking issue was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: widening the form read path to support 64-bit row identifiers. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 9.09% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 11 functions across 12 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
form-read-path-uint64-rows
- Resolve merge conflict in branch
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1022 +/- ##
==========================================
- Coverage 89.58% 89.55% -0.03%
==========================================
Files 177 177
Lines 6010 5996 -14
Branches 994 994
==========================================
- Hits 5384 5370 -14
Misses 397 397
Partials 229 229
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…ly at the ROOT call; FORM's row stays std::uint64_t. test(form): compare 64-bit rows without int casts: get_index() now returns std::uint64_t; drop the int casts from the merged Framework-R-D#992 tests, which fail GCC's -Werror=sign-compare. Split fundamental buffer allocation out of TBranch read(), Fixes readability-function-size on read(), whose signature this PR changes. No behavior change.
Summary
Widen the FORM read path to support 64-bit row IDs consistently across storage interfaces and ROOT backends, matching the existing write path.
Change
get_index(),read(), andentries()to usestd::uint64_tinstead of narrowing row IDs to int in the read path.inttostd::uint64_t. This includesget_index(), index-map values, and read-containerread()andentries()interfaces.LoadTreeasLong64_t.