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

Make opts const, allowing literal parameter. #179

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

shangjiaxuan
Copy link
Contributor

@shangjiaxuan shangjiaxuan commented Oct 13, 2023

The following code currently fails compile because function parameter opts is declared as non-const, disallowing literal constants.

	nifti_image* img = NULL;
	znzFile file = nifti_image_open("image.nii", "rb", &img);

@seanm
Copy link
Contributor

seanm commented Oct 25, 2023

@shangjiaxuan this would be a change to public API and so needs to be done carefully. The maintainers are conservative about changing public API. It might be best to do an audit of where const is missing and batch all such changes together. Similar to #165

@shangjiaxuan
Copy link
Contributor Author

@seanm after searchin through the source code, I believe this is the only function that needs const added. In fact the corresponding function in nifti1_io.h and nifti1_io.c is declared and implemented as const. The use of opts is for the fopen("file", "rb")'s "rb" mode parameter, and literal is expected on same code path.

@seanm
Copy link
Contributor

seanm commented Oct 30, 2023

@shangjiaxuan oh, indeed that is weird that it is const in nifti1_io.c but not in nifti2_io.c. I would have expected the opposite.

@hjmjohnson @afni-rickr given the above, I vote for merging this. What do you think?

@afni-rickr
Copy link
Contributor

I do not see a compilation failure for that, so it presumably depends on build options. But this seems like a fine update.
Thanks @shangjiaxuan.

@afni-rickr afni-rickr merged commit da476fd into NIFTI-Imaging:master Oct 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants