Skip to content

Rewrite rd_kw_type as a c++ class rd::KW - #1295

Merged
eivindjahren merged 10 commits into
mainfrom
fix_things2
Oct 8, 2026
Merged

Rewrite rd_kw_type as a c++ class rd::KW#1295
eivindjahren merged 10 commits into
mainfrom
fix_things2

Conversation

@eivindjahren

@eivindjahren eivindjahren commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This includes a number of minor changes user facing changes.

  • "strided_copy" (now a copy constructor) now creates an empty keyword rather than returning nullptr when appropriate.
  • Constructing a ResdataKW with negative size raises TypeError rather than ValueError.

The goals of this PR is to:

  1. Make it explicit that data for rd::KW is initialized when you have a rd::KW. This is achieved by letting rd::FileKW hold std::variant<rd::KWHeader, rd::KW> so it is clear whether data is initialized (holds alternative rd::KW) or not.
  2. Avoid undefined behavior regarding casting char* to int* in the handling of rd::KW::data. This is achieved by holding a variant of different vectors for data intead.
  3. Update to using size_t for the indices of rd::KW.

Resolves #1087

@eivindjahren
eivindjahren force-pushed the fix_things2 branch 8 times, most recently from 2a9e7f7 to c4328b3 Compare September 28, 2026 10:08
@eivindjahren
eivindjahren requested a review from ajaust September 28, 2026 10:29
@eivindjahren eivindjahren moved this to Ready for Review in SCOUT Sep 28, 2026
@ajaust ajaust self-assigned this Oct 5, 2026
ajaust
ajaust reviewed Oct 6, 2026
View reviewed changes

@ajaust ajaust left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work (and so much of it 🤯).

I am not 100% sure that I got everything, but I did my best to add meaningful comments. Some of the comments will not be relevant for the final state of the PR, but I went through everything commit by commit.

There are a few cases where we may end up with bad memory or out-of-bounds access if I understood it correctly. It is quite hard to follow what size is coming from where and I don't fully know how the state on disk may differ from the state in memory.

Comment thread tests/rd_tests/test_rd_kw.py
Comment thread tests/rd_tests/test_rd_kw.py Outdated
Comment thread tests/rd_tests/test_rd_kw.py Outdated
Comment thread tests/rd_tests/test_rd_kw.py Outdated
Comment thread tests/rd_tests/test_rd_kw.py Outdated
Comment thread lib/include/resdata/rd_kw.hpp Outdated
Comment thread lib/include/resdata/rd_kw_header.hpp Outdated
Comment thread lib/include/resdata/rd_kw_header.hpp Outdated
Comment thread lib/resdata/rd_file_kw.cpp Outdated
Comment thread lib/resdata/rd_file_kw.cpp Outdated
ajaust
ajaust reviewed Oct 7, 2026
View reviewed changes

@ajaust ajaust left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes and nice use of fixup commits. It was very simple to map a fixup to the corresponding comment(s).

I think there is a potential regression for catching errors for fread.

I resolved all comments that were addressed. I think there were one or two original comments by me that were missed or I missed their fixup (at least this comment could still be relevant).

I left the comments addressing transient issues open, but I'm happy to do some squashing and/or maybe adding a comment to the commit messages of commits that introduce an issue that is later resolved. Fixing the intermediate commits may otherwise lead to a bunch of conflicts with the later commits.

Comment thread lib/resdata/rd_kw.cpp Outdated
Comment thread lib/resdata/rd_file.cpp Outdated
Comment thread lib/include/resdata/rd_kw.hpp Outdated
Comment thread lib/resdata/rd_kw.cpp
using VecT = std::decay_t<decltype(vec)>;
for (size_t index = 0; index < index_map.size(); index++) {
int element_index = index_map[index];
if (element_index < 0 || element_index >= element_count)

@ajaust ajaust Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with fixing it in another PR. 👍

@eivindjahren
Fix warnings on gcc and enable WError (gcc only)
798320a 
This also fixes an outdated comment about using warnings as errors
if not on windows.

The warning fixed in rd_kw.cpp was writing to an object with no trivial
copy-assignment. Changed to use copy-initialization.

The warning in test_fortio.cpp was about ignoring attributes on template
argument which was fixed by using a lambda.

The warning in test_well_keyword_validation.cpp was comparison of
expressions of different signdness which was fixed by using size_t for
num_wells.
@eivindjahren
Extend ResdataKW tests
220df42 
This is to support refactoring of ResdataKW to c++ style
code.
@eivindjahren
Extend FortIO tests
d8b2973
@eivindjahren
Compute ResdataKW.sum with numpy
94c6611 
The int, float, double and bool cases all reduce to a sum over the
numpy view of the keyword, so the dedicated _int_sum and _float_sum
are not needed. Booleans are summed as int32 to keep
returning a count rather than a bool.
@eivindjahren
Remove unused rd_kw functions and prototypes
1b38646 
Declarations in rd_kw.hpp without a definition:

  * rd_kw_assert_numeric
  * rd_kw_assert_binary
  * rd_kw_assert_binary_numeric
  * rd_kw_assert_binary_int/_float/_double

Functions that were never called:
  * rd_kw_alloc_scatter_copy
  * rd_kw_content_equal
  * rd_kw_fseek_kw
  * rd_kw_fskip
  * rd_kw_iget
  * rd_kw_max_min

helpers only called through these removed functions:
 * rd_kw_scalar_set__
 * rd_kw_string_eq are removed

Tests for the removed functions are removed as well.
@eivindjahren
Remove unused well_segment_well_is_MSW
872e596
@eivindjahren
Remove unused ERT::ResdataKW<T>
f65f311
@eivindjahren
Remove unused rd_kw_icmp_string
84a0050
@eivindjahren
Remove unused rd_kw_summarize
eb40464
@eivindjahren
Make rd_kw into rd::KW and introduce KWHeader for FileKW
5619a6f 
Also uses std::vector to hold the rd::KW data, avoiding
strict aliasing rules viloations.

Has two user facing changes 1) the pybind function _alloc_new
now takes size_t resulting in a TypeError for negative size rather
than the old ValueError. 2) Failing to read the header now
results in RuntimeError

Fixes three bugs:
-----------------

1) Fixes potential alias of EOF in skip_space_until_quote

2) Fix a bug in in reading of zero sized strings

It would never check the last separator so it would succeed in reading
%0c but then not be placed at the last quote.

3) Makes the count check in sub copy resistant to overflow

Since we already check for offset >= other.size(), other.size() - offset
will not underflow. This way, we avoid count + offset overflowing.

Also Removes C++ tests superseded by the Python tests
-----------------------------------------------------

The following tests should make up for the last coverage:

resdata/tests/rd_kw_equal.cpp:
  test_that_equal_distinguishes_keywords_differing_in_a_single_element
  test_that_message_keywords_compare_equal_when_headers_match

resdata/tests/rd_kw_init.cpp
  superseded by tests/test_rd_kw.cpp

test_truncated in resdata/tests/rd_kw_fread.cpp:
  test_that_short_data_section_raises_value_error
  test_that_mismatch_in_end_record_raises_value_error
  test_that_oversized_record_size_raises_value_error
  test_fortio.py::test_that_reading_a_truncated_record_fails

For the following tests, the python code dispatches on the
type so what is being tested is moot:
 * "scalar_set/scale/shift validate the type"
 * "inplace unary ops validate type"
 * "indexed inplace/copy ops validate size and type"
 * "element sum validates type"
ajaust
ajaust approved these changes Oct 8, 2026
View reviewed changes

@ajaust ajaust left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. 👍

@eivindjahren
eivindjahren merged commit d9eacc1 into main Oct 8, 2026
10 checks passed
@eivindjahren
eivindjahren deleted the fix_things2 branch October 8, 2026 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ajaust ajaust ajaust approved these changes

Assignees

@ajaust ajaust

Labels

None yet

Projects

SCOUT
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

rd_kw_struct uses char *data which is UB.

2 participants

@eivindjahren @ajaust