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

Add const to cmp_value parameters of vector synchronization apis #543

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

stewartl318
Copy link
Collaborator

@stewartl318 stewartl318 commented Sep 9, 2024

Summary of changes

Add const to *cmp_values in vector synchronization apis

Proposal Checklist

  • Link to issue(s)
  • Changelog entry
  • Reviewed for changes to front matter
  • Reviewed for changes to back matter

@stewartl318
Copy link
Collaborator Author

Issue #542

@stewartl318
Copy link
Collaborator Author

PR should be rebased onto rc1

@jdinan jdinan added this to the OpenSHMEM 1.6 milestone Sep 27, 2024
@davidozog
Copy link
Collaborator

Question from WG: add this as new function signature for v1.6 - or add as an errata for v1.5?

@jdinan
Copy link
Collaborator

jdinan commented Oct 10, 2024

This reference may be help with regard to C backwards compatibility: https://stackoverflow.com/questions/5083765/does-changing-fmystruct-a-to-fconst-mystruct-a-breaks-api-abi-in-c/5083976#5083976

As discussed, it may break tools that aren't updated with const as the call to the tool's binding for the function can potentially discard const from the argument passed by the application, which causes a compile time error.

A potentially bigger issue is with type-generic C++ bindings (which SOS has). The function name mangling will be different for the updated function and this will cause ABI breakage. Type-generic C++ bindings would need to add both const and non-const versions of the function. This isn't an issue in C because generic type selection simply chooses an ordinary C function name for one of the existing non-generic functions.

@jdinan jdinan mentioned this pull request Oct 11, 2024
@jdinan jdinan merged commit 02e905a into openshmem-org:master Nov 1, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Sync, Order, Lock (9.11-9.13)
Development

Successfully merging this pull request may close these issues.

3 participants