CEED / libCEED

CEED Library: Code for Efficient Extensible Discretizations
https://libceed.org
BSD 2-Clause "Simplified" License
201 stars 47 forks source link

Get* Interface Uniformity #1700

Open jeremylt opened 2 days ago

jeremylt commented 2 days ago

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 commented 2 days ago

@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 commented 2 days ago

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 commented 2 days ago

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 commented 2 days ago

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)