Skip to content

WIP: autocxx #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

WIP: autocxx #19

wants to merge 23 commits into from

Conversation

mkovaxx
Copy link
Owner

@mkovaxx mkovaxx commented Apr 23, 2025

Just for easy comparison with main.

Chris00 and others added 23 commits February 6, 2025 22:02
The environment variable DEP_MFEM_ROOT has been renamed MFEM_DIR as
this seems to be the expected name https://mfem.org/building/
Autocxx output cargo directives of the form cargo::* which require
that version of Rust.
The environment variable DEP_MFEM_ROOT has been renamed MFEM_DIR as
this seems to be the expected name https://mfem.org/building/
@@ -18,6 +16,6 @@ file (WRITE ${CMAKE_BINARY_DIR}/mfem_info.txt
"INCLUDE_DIRS=${MFEM_INCLUDE_DIRS}\n"
"LIBRARY_DIR=${MFEM_LIBRARY_DIR}\n"
"CXX_FLAGS=${MFEM_CXX_FLAGS}\n"
)
)
Copy link
Owner Author

Choose a reason for hiding this comment

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

nit

@@ -0,0 +1,76 @@
// Disable the surious warnings for the mfem header file.
Copy link
Owner Author

Choose a reason for hiding this comment

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

typo

Suggested change
// Disable the surious warnings for the mfem header file.
// Disable the spurious warnings for the mfem header file.


#define SUBCLASS(A, B) \
/* Functions to cast A to a superclass B. */ \
/* In C++ so errors can be caught by the type system */ \
Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

Comment on lines +62 to -84
let mut mesh_fec = mesh.with_fec(|fec| {
if args.order > 0 {
H1_FECollection::new(args.order, dim).into()
} else if let Some(fec) = fec {
fec.into()
} else {
H1_FECollection::new(1, dim).into()
}
};
Copy link
Owner Author

Choose a reason for hiding this comment

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

This pattern is much cleaner than the original mess that was here.
Thanks! :)

/// `FiniteElementCollection` returned by `fec` is the one that
/// will be used to discretize the PDE.
#[must_use]
pub fn with_fec<'a, FEC>(&'a mut self, fec: FEC) -> MeshWithFEC<'a>
Copy link
Owner Author

Choose a reason for hiding this comment

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

Suggested change
pub fn with_fec<'a, FEC>(&'a mut self, fec: FEC) -> MeshWithFEC<'a>
pub fn map_fec<'a, FecFunc>(&'a mut self, f: FecFunc) -> MeshWithFEC<'a>

The name with_fec feels odd, because all other with_$X functions are part of a "builder pattern". This one is the odd one out, for two main reasons.

First, all other "builder pattern" functions return an entirely independent new thing. This one returns something that holds a reference to the original.

Second, all other "builder pattern" functions take a plain-old-data value that is meant to replace the original value, without regard to what it was. This one processes the original value to produce the new value.

Mostly for the second reason, a name of map_$X form seems easier to understand.

@mkovaxx mkovaxx requested a review from Chris00 May 5, 2025 04:02
@mkovaxx
Copy link
Owner Author

mkovaxx commented May 5, 2025

@Chris00 I just realized I had made a mistake opening this - GitHub doesn't allow the opener to accept a PR... 🙈
Please take a look at my comments here anyway.

Last item I can think of before we merge autocxx is to port example0, as you suggested earlier.

@mkovaxx mkovaxx self-assigned this May 5, 2025
@Chris00
Copy link
Collaborator

Chris00 commented May 5, 2025 via email

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