No manual memory management in rd grid#1145
Conversation
There was a problem hiding this comment.
Pull request overview
This PR modernizes the C++ test/utility surface around rd_grid (and related types) by introducing RAII ownership helpers (to avoid manual *_free calls) and by restructuring the previously monolithic grid tests into smaller, more focused Catch2 test files.
Changes:
- Add
std::unique_ptr-based aliases andmake_*factory helpers for several C-style alloc/free APIs (e.g.,rd_kw,rd_grid, geometry types). - Split and expand
rd_gridtest coverage into multiple dedicated test files (regular grids, xyz lookup, LGR/NNC, coarse groups, compare, misc I/O, LGR naming). - Update multiple APIs and internal structs to take
rd_grid_type*instead ofconst rd_grid_type*(propagating constness changes through callers).
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/tests/test_rd_grid_xyz_lookup.cpp | New tests for rd_grid_get_global_index_from_xyz edge cases (degenerate/twisted/concave/collapsed/flat cells, start-hint behavior). |
| lib/tests/test_rd_grid_regular.cpp | New “regular grid” baseline tests (dimensions, geometry, I/O, kw allocation, export, containment, etc.). |
| lib/tests/test_rd_grid_nnc_lgr.cpp | New tests covering LGR loading, nested LGRs, MAPAXES, NNC variants, and dual-porosity fracture index behavior. |
| lib/tests/test_rd_grid_misc.cpp | New miscellaneous grid tests for file existence, writing/reading, GRDECL printing, and keyword defaults for inactive cells. |
| lib/tests/test_rd_grid_lgr_names.cpp | New tests pinning LGR naming/parenting behavior for EGRID/GRID loaders and lookup semantics. |
| lib/tests/test_rd_grid_compare.cpp | New tests for rd_grid_compare across different grid types/features (actnum/geometry/NNC/coarse/LGR/dual-porosity). |
| lib/tests/test_rd_grid_coarse.cpp | New tests for coarse groups (CORSNUM) including sparse group numbering. |
| lib/tests/test_rd_grid.cpp | Removes the previous monolithic grid test file (replaced by the new split test suite). |
| lib/tests/test_nnc_info.cpp | Extracts/rewrites NNC info/vector tests using RAII wrappers. |
| lib/tests/test_layer.cpp | Refactors test helper to use rd_kw_ptr/rd_grid_ptr RAII instead of manual alloc/free. |
| lib/tests/test_geometry.cpp | Switches geometry tests to RAII helpers (make_geo_*) instead of manual *_alloc/free. |
| lib/tests/test_fault_block_layer.cpp | Updates helpers to use rd_grid_ptr/make_rd_kw and adapts to new grid constness. |
| lib/tests/grid_fixtures.hpp | Adds shared grid/NNC/LGR/coarse/MAPAXES fixtures + file writers; centralizes RAII-based helpers. |
| lib/resdata/tests/rd_region.cpp | Updates test helper signatures to match new rd_grid_type* usage. |
| lib/resdata/tests/rd_grid_layer_contains.cpp | Updates test helper signature to accept rd_grid_type*. |
| lib/resdata/tests/rd_grid_init_fwrite.cpp | Updates helper signatures to accept rd_grid_type*. |
| lib/resdata/tests/rd_grid_copy.cpp | Updates helper signature to accept rd_grid_type*. |
| lib/resdata/tests/rd_fault_block_layer_equinor.cpp | Updates helper signature to accept rd_grid_type*. |
| lib/resdata/tests/rd_fault_block_layer.cpp | Updates helper signatures to accept rd_grid_type*. |
| lib/resdata/rd_subsidence.cpp | Changes rd_subsidence_alloc to accept rd_grid_type*. |
| lib/resdata/rd_region.cpp | Changes stored grid pointer and rd_region_alloc to accept rd_grid_type*. |
| lib/resdata/rd_kw.cpp | Adds make_rd_kw() factory returning rd_kw_ptr. |
| lib/resdata/rd_grid_cache.cpp | Updates cache constructor to accept rd_grid_type*. |
| lib/resdata/rd_grav.cpp | Changes rd_grav_alloc to accept rd_grid_type*. |
| lib/resdata/nnc_info.cpp | Comment clarification for lgr_list contents. |
| lib/resdata/fault_block_layer.cpp | Changes layer to store rd_grid_type*; updates alloc/getter signatures. |
| lib/resdata/fault_block.cpp | Changes fault_block to store rd_grid_type*. |
| lib/private-include/detail/resdata/rd_grid_cache.hpp | Updates rd_grid_cache to hold rd_grid_type*. |
| lib/include/resdata/rd_subsidence.hpp | Public header change for rd_subsidence_alloc(rd_grid_type*). |
| lib/include/resdata/rd_region.hpp | Public header change for rd_region_alloc(rd_grid_type*). |
| lib/include/resdata/rd_kw.hpp | Adds rd_kw_ptr alias + make_rd_kw declaration for C++ users. |
| lib/include/resdata/rd_grid.hpp | Changes constness of several APIs; adds rd_grid_ptr + make_rectangular_grid/read_grid C++ helpers. |
| lib/include/resdata/rd_grav.hpp | Public header change for rd_grav_alloc(rd_grid_type*). |
| lib/include/resdata/rd_file.hpp | Adds rd_file_view_ptr alias for C++ users. |
| lib/include/resdata/rd_coarse_cell.hpp | Adds rd_coarse_cell_ptr alias for C++ users. |
| lib/include/resdata/fault_block_layer.hpp | Public header changes for fault_block_layer_alloc and fault_block_layer_get_grid. |
| lib/include/ert/geometry/geo_surface.hpp | Adds geo_surface_ptr alias + make_geo_surface factory declaration. |
| lib/include/ert/geometry/geo_region.hpp | Adds geo_region_ptr alias + make_geo_region factory declaration. |
| lib/include/ert/geometry/geo_polygon.hpp | Adds geo_polygon_ptr alias + make_geo_polygon factory declaration. |
| lib/include/ert/geometry/geo_pointset.hpp | Adds geo_pointset_ptr alias + make_geo_pointset factory declaration. |
| lib/geometry/geo_surface.cpp | Implements make_geo_surface. |
| lib/geometry/geo_region.cpp | Implements make_geo_region. |
| lib/geometry/geo_polygon.cpp | Implements make_geo_polygon and adds missing <string> include. |
| lib/geometry/geo_pointset.cpp | Implements make_geo_pointset. |
| lib/CMakeLists.txt | Updates rd_test_suite sources to use the new split test files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b4c2a2f to
2a00ce8
Compare
2a00ce8 to
9c5925e
Compare
9c5925e to
5038d13
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
lib/resdata/rd_region.cpp:106
rd_regionis documented as holding a shared reference to a grid which is not modified, and the code here appears to only query the grid. Changingparent_gridandrd_region_allocto take/store a non-constrd_grid_type*makes the API more permissive and is a breaking change for callers withconst rd_grid_type*. Consider revertingparent_gridandrd_region_allocback toconst rd_grid_type*unless mutation is required.
int grid_nx, grid_ny, grid_nz, grid_vol, grid_active;
rd_grid_type *parent_grid;
};
UTIL_IS_INSTANCE_FUNCTION(rd_region, RD_REGION_TYPE_ID)
UTIL_SAFE_CAST_FUNCTION(rd_region, RD_REGION_TYPE_ID)
static void rd_region_invalidate_index_list(rd_region_type *region) {
region->global_index_list_valid = false;
region->active_index_list_valid = false;
}
rd_region_type *rd_region_alloc(rd_grid_type *rd_grid, bool preselect) {
rd_region_type *region = (rd_region_type *)util_malloc(sizeof *region);
UTIL_TYPE_ID_INIT(region, RD_REGION_TYPE_ID);
region->parent_grid = rd_grid;
rd_grid_get_dims(rd_grid, ®ion->grid_nx, ®ion->grid_ny,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e19dd56 to
294e6eb
Compare
This changes semantics slightly so that name is never NULL. The name being NULL is not representable in GRID or EGRID files and it would crash in `rd_grid_add_lgr` as LGR_hash us a map from std::string to rd_grid_type.
Note that this makes rd_grid_init_mapaxes_data_double not assign to the double* instead of dereferencing NULL. This is still not perfect but it is only used in tests and we should remove it in a separate step.
This turned into fixing a lot of const as it was thrown away by rd_grid_get_cell and assert_center would actually mutate cell which meant that many things thought to be const was not.
294e6eb to
4e235e7
Compare
ajaust
left a comment
There was a problem hiding this comment.
Great work! I have a few comments, but nothing too serious. 🙂
|
|
||
| geo_region_ptr make_geo_region(const geo_pointset_ptr &pointset, | ||
| bool preselect) { | ||
| return {geo_region_alloc(pointset.get(), preselect), &geo_region_free}; |
There was a problem hiding this comment.
We might need a comment in the header to point out that the region is borrowing the pointer of the pointset. This is relevant because it implies that the pointset must outlive the region or otherwise the region will have a dangling pointer reference.
Alternatively, we could make sure that geo_region_alloc makes a copy of the pointset.
There was a problem hiding this comment.
Added a comment, I think that is sufficient for now as this was always a problem. I think it would make more sense to use a shared_ptr to really fix it.
There was a problem hiding this comment.
Yeah, a shared_ptr would be a good idea.I am happy with the current documentation by the comment if you prefer to properly fix this in the another PR.
There was a problem hiding this comment.
Ok, lets keep it mind for later then.
4e235e7 to
bb05a96
Compare
No description provided.