Repository navigation
Use size_t for indices in summary - #1306
Merged
Merged
Conversation
eivindjahren
force-pushed
the
size_t_summary
branch
2 times, most recently
from
October 26, 2026 15:19
fd13b5d to
7ab38d9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unsigned report-step overflow can access invalid memory, and sparse report-only vectors retain artificial trailing entries.
2 open issues
What changed in this PR
Migrates summary indices and counts to size_t across the C++ implementation, public interfaces, and Python bindings.
Changes:
- Replaces negative index sentinels with optional values while preserving Python’s
-1lookup results. - Improves handling of empty summaries, missing reports, and invalid indices.
- Expands regression coverage for summary loading, writing, interpolation, and errors.
| File | Description |
|---|---|
| tests/rd_tests/test_sum.py | Adds broad summary regression coverage. |
| tests/rd_tests/test_rd_sum.py | Tests negative-index binding errors. |
| python/resdata/summary/rd_sum_keyword_vector.py | Rejects excessively negative indices. |
| lib/tests/test_time_index.cpp | Removes signed report-step tests. |
| lib/tests/test_rd_sum.cpp | Updates index types and optional-result tests. |
| lib/tests/test_fortio.cpp | Removes negative-count test. |
| lib/resdata/well_rseg_loader.cpp | Uses unsigned keyword index maps. |
| lib/resdata/smspec_node.cpp | Migrates parameter index constructors. |
| lib/resdata/rd_unsmry_loader.cpp | Migrates loader indices and counts. |
| lib/resdata/rd_sum.cpp | Updates summary APIs and lookup results. |
| lib/resdata/rd_sum_vector.cpp | Stores optional parameter indices. |
| lib/resdata/rd_sum_vector_pybind.cpp | Accepts unsigned keyword indices. |
| lib/resdata/rd_sum_tstep.cpp | Migrates timestep indices and write maps. |
| lib/resdata/rd_sum_pybind.cpp | Updates bindings and preserves lookup sentinels. |
| lib/resdata/rd_sum_file_data.cpp | Updates report handling and corrects first-day units. |
| lib/resdata/rd_sum_data.cpp | Migrates case indexing and adds bounds checks. |
| lib/resdata/rd_smspec.cpp | Updates parameter maps and size tracking. |
| lib/resdata/rd_kw.cpp | Uses unsigned indexed-read arguments. |
| lib/resdata/rd_file_view.cpp | Updates keyword and summary indices. |
| lib/resdata/FortIO.cpp | Uses unsigned seek counts. |
| lib/private-include/detail/resdata/rd_unsmry_loader.hpp | Updates loader declarations and storage. |
| lib/private-include/detail/resdata/rd_sum_file_data.hpp | Introduces optional report ranges. |
| lib/include/resdata/smspec_node.hpp | Exposes unsigned parameter indices. |
| lib/include/resdata/rd_sum.hpp | Updates public summary signatures. |
| lib/include/resdata/rd_sum_vector.hpp | Updates keyword-vector signatures. |
| lib/include/resdata/rd_sum_tstep.hpp | Updates timestep access and writing signatures. |
| lib/include/resdata/rd_sum_data.hpp | Updates data APIs and optional lookups. |
| lib/include/resdata/rd_smspec.hpp | Updates metadata indices and mapping interfaces. |
| lib/include/resdata/rd_kw.hpp | Updates indexed-read declarations. |
| lib/include/resdata/rd_file.hpp | Updates summary-view index type. |
| lib/include/resdata/rd_file_view.hpp | Updates indexed-loading interfaces. |
| lib/include/resdata/FortIO.hpp | Updates seek argument declarations. |
| lib/CMakeLists.txt | Removes the deleted test source. |
Review effort: Balanced
eivindjahren
force-pushed
the
size_t_summary
branch
2 times, most recently
from
October 27, 2026 06:40
0b7b7bf to
9e54325
Compare
ajaust
self-requested a review
October 27, 2026 07:56
ajaust
reviewed
Oct 27, 2026
ajaust
left a comment
ajaust
left a comment
Collaborator
There was a problem hiding this comment.
Nice! I have some smaller comments. 🙂
Merged
eivindjahren
force-pushed
the
size_t_summary
branch
from
October 28, 2026 06:45
ecff443 to
ca42890
Compare
ajaust
approved these changes
Oct 28, 2026
ajaust
left a comment
ajaust
left a comment
Collaborator
There was a problem hiding this comment.
Nice! One additional comment to raise awareness, but looks good overall. 👍
| template <typename ReportNr = int> | ||
| std::filesystem::path filename(std::filesystem::path path, FileType, | ||
| bool fmt_file, int report_nr = -1); | ||
| bool fmt_file, ReportNr report_nr = -1); |
Collaborator
There was a problem hiding this comment.
I see a slight risk that we get an underflow if we would call
filename<std::size_t>(path, type, fmt)I don't think it is a huge risk now, but I wanted to mention it.
Collaborator
Author
There was a problem hiding this comment.
Well, its an underflow which is then formatted as size_t_max so just a weird looking filename :D
Integers read from file are validated as non-negative before being converted to size_t. fread_indexed_data now takes a vector<size_t> index_map, and pybind summary methods take size_t so negative values raise TypeError. The ministep number is purely informational and is kept as the int read from file. This also fixes some existing bugs: - Looking up a report step by days or time no longer loops forever when a report step in the range is missing. - Empty data in the summary index and file data (length, first_report, last_report, lookup_time, lookup_days) now throws instead of causing undefined behaviour. - rd_sum_data_iget checks that params_index is in range and throws std::out_of_range instead of reading out of bounds. - Report-only time and data vectors skip report steps missing from the data instead of reading the invalid range of a missing report, and are sized by the values actually written, so they no longer end with a spurious zero entry when report steps are missing. This replaces the report_only flag of the rd_sum_data allocators with separate report functions. - Adding a time step with report step SIZE_MAX throws std::invalid_argument instead of wrapping report_step + 1 to zero and indexing outside the report map.
This would cause size_t underflows.
eivindjahren
force-pushed
the
size_t_summary
branch
from
October 28, 2026 07:17
ca42890 to
2094f4f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.