Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Create CeedOperatorFieldGet*Read routines with const parameters #1639

Closed
jrwrigh opened this issue Jul 24, 2024 · 4 comments · Fixed by #1646
Closed

Create CeedOperatorFieldGet*Read routines with const parameters #1639

jrwrigh opened this issue Jul 24, 2024 · 4 comments · Fixed by #1646
Labels
interface

Comments

@jrwrigh
Copy link
Collaborator

jrwrigh commented Jul 24, 2024

I think on the existing CeedOperatorFieldGet* probably aren't appropriate for "normal" uses by end-users; Those objects shouldn't be editable (or at least Destroyable), but the lack of a const qualifier on the parameters means you can. See HONEE MR for example of accidentally Destroying something gotten from CeedOperatorFieldGet*.

Adding const qualifiers would break existing code unfortunately, so assuming that's a non-starter, maybe we could implement "read" versions of the Getters?
i.e.:

CEED_EXTERN int CeedOperatorFieldGetElemRestrictionRead(CeedOperatorField op_field, const CeedElemRestriction *rstr);
CEED_EXTERN int CeedOperatorFieldGetBasisRead(CeedOperatorField op_field, const CeedBasis *basis);
CEED_EXTERN int CeedOperatorFieldGetVectorRead(CeedOperatorField op_field, const CeedVector *vec);
CEED_EXTERN int CeedOperatorFieldGetDataRead(CeedOperatorField op_field, const char **field_name, const CeedElemRestriction *rstr, const CeedBasis *basis,
                                         const CeedVector *vec);
@jeremylt
Copy link
Member

I don't love this part of the API as it is. We break the get-restore pattern we have with other proper objects. It might be disruptive, but I think forcing this API to match our get-restore pattern. The get would clone a reference and the restore would destroy that reference?

I'm hesitant to make "Read" versions since the Vector is still inherently mutable so its sorta misleading.

@jrwrigh
Copy link
Collaborator Author

jrwrigh commented Jul 30, 2024

Yeah, I think adding the Restore-half of the Get methods works well too.

@jeremylt
Copy link
Member

We'd have to spin up a companion PR to MFEM since it will disrupt them a little, but it feels like the "right" way to do that interface.

@jeremylt
Copy link
Member

Ugh, this becomes much messier that I hoped. There are 735 instances of CeedOperatorFieldGet* in the codebase, and updating the usage for OCCA is a hassle. This would help our Rust usage though, especially if we add CeedOperatorRestoreFields. This sorta has to be done in one big bite, so this branch is slow going.

@jeremylt jeremylt mentioned this issue Aug 15, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interface
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Must Destroy OperatorField Objects CEED/libCEED
2 participants
@jrwrigh @jeremylt