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

Implemented safer C++ DataContainer data type handling, fixes #1193 #1195

Closed
wants to merge 24 commits into from

Conversation

evgueni-ovtchinnikov
Copy link
Contributor

Changes in this pull request

Current C++ DataContainer classes use static casts of void* pointers, which may be unsafe (cf. #1193).

This PR implements an alternative template-based design whereby static casts are only used on C interface level, which is not supposed to be used by SIRF users.

Testing performed

Related issues

Fixes #1193

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have added docstrings/doxygen in line with the guidance in the developer guide
  • I have implemented unit tests that cover any new or modified functionality
  • The code builds and runs on my machine
  • CHANGES.md has been updated with any functionality change

Contribution Notes

Please read and adhere to the contribution guidelines.

Please tick the following:

  • The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.

Copy link
Member

@KrisThielemans KrisThielemans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some probably mostly random comments but hopefully helpful in this review.

I'm not sure if templating ImageData is a good idea. It certainly breaks backwards compatibility in a huge way. Possibly this could be done by having an intermediate ImageDataTemplate like you did for DataContainer. However, it might not work, as I guess we'd then need to derive ImageDatafromDataContainer, but ImageDataTemplatefromDataContainerTemplate`. This can be done in C++, but has the potential for some confusion.

src/Registration/cReg/NiftiImageData.cpp Outdated Show resolved Hide resolved
src/Registration/cReg/NiftyResampler.cpp Show resolved Hide resolved
src/Registration/cReg/cReg_p.cpp Show resolved Hide resolved
src/common/include/sirf/common/DataContainer.h Outdated Show resolved Hide resolved
src/xGadgetron/cGadgetron/gadgetron_data_containers.cpp Outdated Show resolved Hide resolved
@KrisThielemans KrisThielemans added this to the 3.5 milestone Jun 8, 2023
@KrisThielemans KrisThielemans removed this from the 3.5 milestone Jun 22, 2023
@evgueni-ovtchinnikov
Copy link
Contributor Author

Left unattended for a long time, resulting in weird conflicts with master. Taken over by PR #1210.

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

Successfully merging this pull request may close these issues.

Longer term solutions to DataContainer data type issue needed.
2 participants