Fix rd_grid_alloc_mapaxes_kw not working correctly with missing mapaxes and fix lazy reading of formatted summary files#1196
Conversation
eivindjahren
changed the title
eivindjahren
force-pushed
the
fix_bugs
branch
4 times, most recently
from
d0d9743 to
9b2d214
Compare
ajaust
left a comment
ajaust
left a comment
There was a problem hiding this comment.
Looks good overall. 👍
I have some remarks that should be straighforward to fix. I have some concerns about introducing new calls to util_abort, but I see that this happens in a part of the code where another util_abort exists anyway.
What do we do about this PR
#1134
Will say "thank you" and then close it after your PR is merged?
| const int *actnum); | ||
| rd_kw_type *rd_grid_alloc_volume_kw(const rd_grid_type *grid, bool active_size); | ||
| rd_kw_type *rd_grid_alloc_mapaxes_kw(const rd_grid_type *grid); | ||
| std::optional<rd_kw_ptr> rd_grid_alloc_mapaxes_kw(const rd_grid_type *grid); |
There was a problem hiding this comment.
The commit message says
Fix rd_grid_alloc_mapaxes with for missing mapaxes
I think it was supposed to be
Fix rd_grid_alloc_mapaxes for missing mapaxes
| inline rd_kw_ptr make_rd_kw(const char *header, int size, | ||
| rd_data_type data_type, const void *data) { | ||
| return {rd_kw_alloc_new(header, size, data_type, data), rd_kw_free}; | ||
| } |
There was a problem hiding this comment.
This is for my understanding:
I see that we call rd_kw_alloc and rd_kw_alloc_new which almost the same things, but rd_allow_new will also memcpy the data.
Why do we need the two different ways of making a rd_kw? As we are more leaning towards C++ now, I expected make_rd_kw to get a signature like this:
inline rd_kw_ptr make_rd_kw(const char *header, int size,
rd_data_type data_type, const void *data = nullptr) {
return {rd_kw_alloc_new(header, size, data_type, data), rd_kw_free};
}but I get that this would lead to very different behavior (memcpy_data vs non memcpy).
There was a problem hiding this comment.
Yes, lets refactor this separately.
| rd_kw_type *rd_kw_fread_alloc(ERT::FortIO &); | ||
| rd_kw_type *rd_kw_alloc_actnum(const rd_kw_type *porv_kw, float porv_limit); | ||
| void rd_kw_fread_indexed_data(ERT::FortIO &fortio, offset_type data_offset, | ||
| void rd_kw_fread_indexed_data(ERT::FortIO &fortio, offset_type kw_offset, |
There was a problem hiding this comment.
The commit says
Fix a lazy-loading formatted summaries
but should maybe be
Fix lazy-loading of formatted summaries
| util_abort("%s: failed to load keyword at offset:%ld\n", __func__, | ||
| (long)kw_offset); |
There was a problem hiding this comment.
Are we sure that we want to introduce new util_abort calls? I think this is a problem for users calling using the Python interface as they will not be able to catch this as exception.
Could we throw an exception
| util_abort("%s: failed to load keyword at offset:%ld\n", __func__, | |
| (long)kw_offset); | |
| throw std::ios_base::failure( | |
| fmt::format("failed to load keyword at offset:{}", kw_offset)); |
instead?
There was a problem hiding this comment.
Unfortunately called through code that does not clean up if an exception is thrown so I would rather fix that first before introducing an exception.
| util_abort("%s: Element index is out of range 0 <= %d < %d\n", | ||
| __func__, element_index, element_count); |
There was a problem hiding this comment.
I don't have too strong feelings about this util_abort since that one is preexisting.
2 participants
Resolves #1117
Resolves #901