WIP: More warning fixes

[seanm]

@hjmjohnson @afni-rickr some of these may be more controversial... as they change API or behaviour slightly.

Thoughts?

[seanm]

@afni-rickr care to weigh in on this WIP?

[afni-rickr]

Sorry for being slow. I am not sure how much to go into right now, but these changes are not thrilling to me.

• static functions in main programs: not clear why
• addition of assert: do not really like library deciding to crash
• removal of DATE: why is this important? not everyone uses cmake
• functions returning const char *: feeding this const virus does not seem necessary
• signed/unsigned casting: I do not think those were mistakes I will try to post more specific comments/questions...

[seanm]

static functions in main programs: not clear why

Superficially: to fix -Wmissing-prototypes warnings. It prevents private symbols from being exported, thus allowing some complier/linker optimizations.

addition of assert: do not really like library deciding to crash

There will be no crash in reality, because the default case of that switch statement is impossible. The assert(0) just makes the compiler understand that it's impossible.

removal of DATE: why is this important? not everyone uses cmake

cmake is irrelevant. Removal of __DATE__ is for reproducible builds. Some motivation discussed here for example: https://reproducible-builds.org/

functions returning const char *: feeding this const virus does not seem necessary

:) Yes, const can be like a virus...

But casting away const is a very big foot gun. It might technically be safe in these instances, but as these are the only remaining occurrances of -Wconst-qual warnings, there's no big cascade of additional const to come.

Having a codebase build cleanly with -Wconst-qual I think is a must these days.

signed/unsigned casting: I do not think those were mistakes

Could you elaborate?

[afni-rickr]

Finally going through this in reasonable detail... it seems more clear to group comments here, rather than making either of you search through the list of conversations.

nifti2/clib_02_nifti2.c: show_help() niftilib/nifti_tester001.c: generate_reference_image() Yes, of course, static is fine there. I was thrown by the comments on cleaning up the global namespace (and the translation unit), along with the library functions missing them, and did not even notice that these actually had no prototypes until finally going through this in more detail. Adding actual prototypes at the top might be preferable, but either way is fine.

nifti2/nifti2_io.c: nifti_fileexists() nifti2/nifti2_io.c: nifti_extension_size() Removing symbols from the library could be far more strongly justified as a real bug by someone using them. Removing them breaks backwards compatibility, and it works as it is. Clearly they should have prototypes in any case. For nifti_fileexists(), putting the prototype in nifti2_io.h seems appropriate. That looks like a function people would use. But it is hard to say about nifti_extension_size(). Maybe that function would be okay to remove from the library exports (though it should still get a (static) prototype at the top of nifti2_io.c). Either making it static or putting it in the .h seems okay, though we will have to make a version comment if it is removed from the export list.

nifti2/nifti2_io.c: nifti_find_file_extension() It would be nice to look into this further. Maybe you mean -Wcast-qual here. In any case, I would much rather resolve such warnings with internal casting than to inflict const on the calling packages, speaking as one who has gone through that. Maybe this change should be left out and noted for further investigation. Or we could take the time to figure it out now... time is hard to come by.

nifticdf/nifticdf.c: student_pq2s() Yes indeed, that function looks intended to be local. I would prefer to give it a prototype at the top of nifticdf.c (including static), but that file (from external sources) does not seem to do that for anything. So indeed, just adding 'static' seems fine.

niftilib/nifti1_io.c: nifti_get_filesize() To reduce similar confusion about whether the casts were mistakes in the future, this should probably be done explicitly, as something like:

/* alter size and sign in separate casts to prevent compiler whining */
return (int)(unsigned int)but.st_size;

niftilib/nifti1_io.c: nifti_convert_nhdr2nim() Similar to nifti_get_filesize().

/* alter size and sign in separate casts to prevent compiler whining */
unsigned char c = (unsigned char)*((char *)(&nhdr.qform_code));

niftilib/nifti_tester001.c: PrintTest() Adding static or a prototype is great here, of course. Also, that leading '' may go against the C standard. It looks like the '_' version was added to take the __LINE__ parameter after the fact. So how about this being done with a different name (we tend to add and 'engine' suffix, but whatever is okay): static void PrintTest_eng(...);

Thanks.

[seanm]

1.

nifti2/nifti2_io.c: nifti_fileexists() nifti2/nifti2_io.c: nifti_extension_size() Removing symbols from the library could be far more strongly justified as a real bug by someone using them. Removing them breaks backwards compatibility, and it works as it is.

Is it not exceedingly improbable that anyone is using them, given that there's no declaration in the .h file?

2. I've dropped the -Wcast-qual commit. Will make a separate PR for further discussions.
3. I've dropped the casting changes.
4. I added the PrintTest_eng rename.

[afni-rickr]

nifti2/nifti2_io.c: nifti_fileexists() nifti2/nifti2_io.c: nifti_extension_size() Removing symbols from the library could be far more strongly justified as a real bug by someone using them. Removing them breaks backwards compatibility, and it works as it is.

Is it not exceedingly improbable that anyone is using them, given that there's no declaration in the .h file?

It would be very surprising if no one is using nifti_fileexists(), in particular. Missing a prototype in the .h file is not going to deter someone who wants to use it. I recall many instances of function prototypes being missing from standard (system installable) packages. If it is the function you want to use and it is in the library, you use it.

[hjmjohnson]

@afni-rickr If we want to support nifti_fileexists, then we need to put it in the public interface in the standard way.

Either make it static to declare that it is not available, or make it available in the .h file.

Just because some "can be done" does not mean that we should allow or support it.

[seanm]

Either make it static to declare that it is not available, or make it available in the .h file.

Exactly. Just a question of deciding if it's supposed to be public API or not. Sounds like @afni-rickr is saying it's meant to be public.

[afni-rickr]

Removing symbols should be considered a moderately big deal, as it breaks backward compatibility. For exported functions missing a prototype, the "default" should be to simply add the prototype, not to take them out of the library. And I see no good reason to remove nifti_fileexists(). Even extension_size, it is really preferable to remove it? I am not particularly against that, but anything removed at this point needs a clear entry perhaps in Updates.txt, at least.

[seanm]

I mean, arguing this is not a hill I want to die on, but, in my view, anyone using a function not declared in a public header is asking for trouble and should not expect backwards compatibility when using private API.

I'll note that ITK and libMINC (the only niftilib users I know) are not using it.

Finally, that function should be discouraged anyway, as it's prone to TOCTOU bugs.

[afni-rickr]

I would rather not punish someone for using a function that exists in the library. It is odd that the function tries to actually open the file, rather than simply call stat(), say. But it does not really seem to be a serious security risk, it just opens (for reading) and closes it. If we want to rewrite it using stat(), that would be a separate matter (though it could be done here). This was just supposed to address missing prototypes.

[mrneont]

Hi- In general, when a prototype is missing for a function, it would seem to make sense to resolve the situation by adding the prototype rather than by purging the function. Maintaining backwards compatibility could be considered part of reproducibility, as well?

[seanm]

It is odd that the function tries to actually open the file, rather than simply call stat()

That wouldn't solve the TOCTOU. nifti_fileexists() can truthfully answer yes, then an instant later some other process could delete the file. Whatever answer nifti_fileexists() gives you cannot be assumed to be still true one line of code later.

Such races should be discouraged, and adding this function to the public .h seems to be the opposite.

[seanm]

I've reverted the -Wmissing-prototypes warning fix for nifti_fileexists. This way it's not "removed" from the library, but also not encouraged by being added to the header file.

[afni-rickr]

This is not a semaphore-protected read/write library, where checking is required be atomically combined with reading or writing. The read functions have their own checks for success, as do writes. If a file gets deleted and later operations fail, that is expected behavior. But no big deal, you do not need to add the prototype, that can be done later.