Skip to content

Reduce duplication in Coordinates constructors#3372

Open
ZedThree wants to merge 13 commits into
nextfrom
coordinates-split-ctor
Open

Reduce duplication in Coordinates constructors#3372
ZedThree wants to merge 13 commits into
nextfrom
coordinates-split-ctor

Conversation

@ZedThree

Copy link
Copy Markdown
Member

We had a lot of duplicated complicated code in the Coordinates initialisation, this attempts to reduce it. This should make the refactor in #2873 smaller and easier to review as well

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/mesh/coordinates.cxx
readFromMesh(options, "");
}

Coordinates::Coordinates(Mesh* mesh, Options* options, const CELL_LOC loc,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

warning: constructor does not initialize these fields: non_uniform [cppcoreguidelines-pro-type-member-init]

Coordinates::Coordinates(Mesh* mesh, Options* options, const CELL_LOC loc,
^

Comment thread src/mesh/coordinates.cxx
Comment thread src/mesh/coordinates.cxx Outdated
Comment thread src/mesh/coordinates.cxx Outdated
Comment thread src/mesh/coordinates.cxx Outdated
Comment thread src/mesh/coordinates.cxx Outdated
Comment thread src/mesh/coordinates.cxx Outdated
@ZedThree ZedThree force-pushed the coordinates-split-ctor branch 3 times, most recently from 7aff4fe to 4a08cb3 Compare May 14, 2026 15:54

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread src/mesh/data/gridfromoptions.cxx
@ZedThree ZedThree force-pushed the coordinates-split-ctor branch 2 times, most recently from a976e78 to a3d0999 Compare June 15, 2026 16:36

@dschwoerer dschwoerer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would prefer to revert the last two commits, as it requires as reading the fields twice, so it is imho not that much cleaner then the old design.

Comment thread src/mesh/coordinates.cxx Outdated
@ZedThree

Copy link
Copy Markdown
Member Author

It is a little annoying, but note that we don't actually read twice -- at worst, the data will be read on the first mesh->get call, and we're just paying for an additional copy. And this is only done for two fields during setup, so this shouldn't really affect performance.

Mostly I am now determined that Mesh::get has reached the end of its life and we need to replace it

@ZedThree

Copy link
Copy Markdown
Member Author

@dschwoerer I've reverted those commits

@ZedThree ZedThree force-pushed the coordinates-split-ctor branch from 0401baa to 78e8f0a Compare June 19, 2026 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants