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.

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

Get* Interface Uniformity #1700

Open
jeremylt opened this issue Oct 22, 2024 · 4 comments
Open

Get* Interface Uniformity #1700

jeremylt opened this issue Oct 22, 2024 · 4 comments

Comments

@jeremylt
Copy link
Member

jeremylt commented Oct 22, 2024

Ok, we're pretty inconsistent about when a Get interface needs a Restore/Destroy. Here's the list of ones that don't.

I think an easy point of consistency would be to require a Destroy when the Get returns a Ceed* object.

ceed.h

Function Scalar Quantity Has Restore
CeedBasisGetQRef No No
CeedBasisGetQWeights No No
CeedBasisGetInterp No No
CeedBasisGetInterp1D No No
CeedBasisGetGrad No No
CeedBasisGetGrad1D No No
CeedBasisGetDiv No No
CeedBasisGetCurl No No
CeedQFunctionGetFields No No
CeedQFunctionContextGetAllFieldLabels No No
CeedContextFieldLabelGetDescription No No
CeedOperatorGetFields No No
CeedOperatorGetFieldByName No No
CeedCompositeOperatorGetSubList No No
CeedCompositeOperatorGetSubByName No No
CeedOperatorFieldGetName No No

backend.h

Function Scalar Quantity Has Restore
CeedBasisGetCollocatedGrad No No
CeedQFunctionContextGetFieldLabel No No
CeedOperatorGetQFunctionAssemblyData No No
CeedQFunctionAssemblyDataGetObjects No No
CeedOperatorAssemblyDataGet* No No
CeedOperatorGetFallback No No
CeedOperatorGetFallbackParent No No

String getters I think are fine
(I think they are unlikely to be kept by the user for a long time, and they are constantly, so the user has to mean to bypass that to edit them)

Function Scalar Quantity Has Restore
CeedGetResource No No
CeedGetErrorMessage No No
CeedGetResourceRoot No No
CeedGetOperatorFallbackResource No No
CeedQFunctionGetKernelName No No
CeedQFunctionGetSourcePath No No
CeedQFunctionFieldGetName No No
CeedQFunctionFieldGetData No No

Others I think are fine
(Backend use only, intended to be edited, and should not have multiple access at the same time)

Function Scalar Quantity Has Restore
Ceed*GetData No No
@jeremylt
Copy link
Member Author

@jrwrigh continuing some of our discussions otherwhere.

I think some of these make sense to leave without a Restore, like when we get a const char *? (CeedGetResource for example)

@jrwrigh
Copy link
Collaborator

jrwrigh commented Oct 22, 2024

Yeah, if we can reliably enforce a const to be used by the user, then it doesn't need a restore.

Which I guess we could've done with CeedGetJit* by using const * const char *? Or something like that, I'm pretty sure there's a semantic to declare const for nested pointers.

@jedbrown
Copy link
Member

Note that returning const pointers does not prevent use after free. I don't think we have a good rule for this in PETSc either. It's vaguely "if the caller might be tempted to hang onto the reference but doing so could produce confusing behavior, then a Restore is needed". So returning an immutable string that would only be compared is okay, as is KSPGetPC. I don't think anyone is happy with this vagueness, but it's kind of a hassle to program with explicit restores everywhere.

@jeremylt
Copy link
Member Author

Yeah, I don't think we want restore for all of those. I'm less worried about the strings because those are typically used for display. I'm also less worried about the ceed/backend.h functions.

I think on the whole taking an approach of refcounting any Ceed, CeedVector, ect from these interfaces and the requiring Ceed*Destroy would make things easier for us and would be a good uniformity step, as it imposes the rule that any Ceed object in scope should be destroyed. (Also would help simplify any odd edges in FFI, especially Rust)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants