Skip to content

Clean up rd_grid.cpp some#1118

Merged
eivindjahren merged 3 commits intomainfrom
modernize_for_loops
Feb 12, 2026
Merged

Clean up rd_grid.cpp some#1118
eivindjahren merged 3 commits intomainfrom
modernize_for_loops

Conversation

@eivindjahren
Copy link
Copy Markdown
Collaborator

@eivindjahren eivindjahren commented Feb 11, 2026

Add tests for rd_grid.cpp and modernize the for loops that are trivial.

I added tests for all functions that could be added tests for, which was cut short because of #1117 and that there is no simple way of dealing with coarse cells.

Then I very carefully replaced the for loop patterns that could trivially be replaced.

@eivindjahren eivindjahren changed the title Modernize for loops Clean up rd_grid.cpp some Feb 11, 2026
@eivindjahren eivindjahren force-pushed the modernize_for_loops branch 8 times, most recently from c24ee35 to 55be67f Compare February 11, 2026 11:22
@eivindjahren eivindjahren requested a review from Copilot February 11, 2026 11:24
@eivindjahren eivindjahren self-assigned this Feb 11, 2026
@eivindjahren eivindjahren moved this to Ready for Review in SCOUT Feb 11, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds unit tests around rd_grid utility/file I/O behavior and modernizes several trivial loop patterns in rd_grid.cpp (mostly scoping loop variables).

Changes:

  • Added broad unit tests for regular rectangular grids covering indexing, geometry, keyword allocation, export helpers, and basic file I/O.
  • Modernized a number of for loops in rd_grid.cpp (scoped loop indices and some range-based loops).
  • Removed a couple of declarations from the public rd_grid.hpp header.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
lib/tests/test_rd_grid.cpp Adds extensive Catch2 coverage for many rd_grid APIs plus some smoke tests for grid file writing/loading.
lib/resdata/rd_grid.cpp Refactors multiple loops to modern C++ style (scoped indices / range-based), with no intended behavior change.
lib/include/resdata/rd_grid.hpp Removes two function declarations from the public header (potential API surface change).
Comments suppressed due to low confidence (1)

lib/resdata/rd_grid.cpp:951

  • The range-based loop variable can be const auto &p since p is not modified. This better communicates intent and prevents accidental mutation of corner_list elements.
    for (auto &p : cell->corner_list) {
        if ((p.x == 0) && (p.y == 0)) {
            SET_CELL_FLAG(cell, CELL_FLAG_TAINTED);
            break;
        }
    }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/tests/test_rd_grid.cpp Outdated
Comment thread lib/include/resdata/rd_grid.hpp
Comment thread lib/include/resdata/rd_grid.hpp
Comment thread lib/tests/test_rd_grid.cpp Outdated
Comment thread lib/tests/test_rd_grid.cpp
@eivindjahren eivindjahren force-pushed the modernize_for_loops branch 4 times, most recently from f5245ad to 076cc2d Compare February 11, 2026 12:17
Comment thread lib/resdata/rd_grid.cpp
int coords_size, int **coords, float **corners, const float *mapaxes) {
if (dualp_flag != FILEHEAD_SINGLE_POROSITY)
nz = nz / 2;
{
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not familiar with C++, what is the purpose of these outer curly braces wrapping the whole loop expressions? Is this some old convention?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It creates a scope for the variables inside it. Its still useful for setting when an object is destructed when using RAII in c++

Copy link
Copy Markdown

@SAKavli SAKavli left a comment

Choose a reason for hiding this comment

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

🥓

@github-project-automation github-project-automation Bot moved this from Ready for Review to Reviewed in SCOUT Feb 11, 2026
@eivindjahren eivindjahren merged commit 54ca650 into main Feb 12, 2026
13 checks passed
@github-project-automation github-project-automation Bot moved this from Reviewed to Done in SCOUT Feb 12, 2026
@eivindjahren eivindjahren deleted the modernize_for_loops branch February 12, 2026 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants