-
Notifications
You must be signed in to change notification settings - Fork 45
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
CEED_STRIDES_BACKEND related updates #1463
Conversation
3d137bf
to
7e7845e
Compare
91b4c57
to
830969d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM at a high level. Thanks for solving this problem. I take it we aren't enforcing anything new outside the memcheck backend?
The only thing that is disruptive is f447f4b - really this function signature should have never had a pointer to an array. Its not a big change for users and I think we should fix. We aren't forcing any changes in how the current API behaves past that. |
830969d
to
62c1cb2
Compare
I have
twofour changes hereExpanded note in the documentation about
CEED_STRIDES_BACKEND
Set
CEED_STRIDES_BACKEND
to be different for memcheck than the default layouts for both CPU and GPUAdd
CeedElemRestrictionGetLLayout
for strided restrictionsFix
CeedElemRestrictionGet[L,E]Layout
signature, previously there ways an unneeded pointerThis won't provide a clear warning message, but it will at least demonstrate bugs if a user mixes and matches memcheck
CEED_STRIDES_BACKEND
with other backends.I don't think we can be really any clearer with the name that
CEED_STRIDES_BACKEND
is specific to the backend?Offering
CeedElemRestrictionGetLLayout
probably fixes the root cause of why someone would see this problem though.See #1460
Note: if we like this, a666b53, 0e6bd4c, and 32058e1 should be squashed together
Also note: if we like
CeedElemRestrictionGetLLayout
, I'll go add some quick unit test