Skip to content

Compile finite volume implementation only once - #3405

Merged
ZedThree merged 8 commits into
move-opsfrom
move-ops-impl
Jun 24, 2026
Merged

ZedThree merged 8 commits into
move-opsfrom
move-ops-impl

Conversation

@dschwoerer

Copy link
Copy Markdown
Contributor

The BOUT++ provided slope limiters are all instantiated, using the previous header file.

If a user wants to use a different slope limiter, they can #include the implementation header (or just upstream, as one should).

@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 include/bout/fv_ops_impl.hxx
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated

@ZedThree ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice! Very clever implementation of this.

There's just a few bits of commented out code to clean up, and as we're touching this, may as well fix the clang-tidy warnings. Then we're ratcheting our way to being totally warning clean!

It would also be good to just add something to the docstrings of these functions about how to add a new limiter without upstreaming it.

Comment thread include/bout/fv_ops.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops_impl.hxx Outdated
Comment thread include/bout/fv_ops.hxx Outdated
Comment thread src/mesh/fv_ops.cxx Outdated
@ZedThree

Copy link
Copy Markdown
Member

Please could you just add something about needing to include fv_ops_impl and/or adding an explicit instantiation in order to add a new flux limiter? Then I think this is good to go

@dschwoerer

Copy link
Copy Markdown
Contributor Author

Sorry, forgot about them. Should be good now.

@dschwoerer

Copy link
Copy Markdown
Contributor Author

Now we could also make this a configurable runtime choice.
But I do not feel strongly enough about it to implement it.

@ZedThree ZedThree left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Making it a runtime choice would be neat, but can be done later

@ZedThree
ZedThree merged commit e00a0a0 into move-ops Jun 24, 2026
29 of 30 checks passed
@ZedThree
ZedThree deleted the move-ops-impl branch June 24, 2026 16:05
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