Compile errors on windows due to changes in nii_dicom_batch.cpp

[ljod]

Hi and thank you very much for the recently added nrrd support! We have found that it doesn't yet compile on Windows (in the context of Slicer nightly multi-platform builds that are building dcm2niix) due to some minor issues with abs (needs to be changed to fabs) and getline (could be fixed by updating the code to use istream::getline instead). The needed fixes and the errors are described here:

https://discourse.slicer.org/t/slicerdcm2nii-extension-not-visible-in-extension-manager-despite-successful-build/6435/2

I don't have a Windows machine so I'm posting this here in the hopes that these minor changes are really easy for someone else. Thanks!!!

[neurolabusc]

@fedorov can you provide some input here. The getline call supports a feature you requested but I do not think anyone uses. In order to support many Windows builds, the simple fix is to add a dependency to iostream. At the moment, iostream is only required if the user compiles with CharLS support (and CharLS also requires C++14).

I prefer to have the minimum number of dependencies for the basic build. @fedorov would you mind if your feature request 252 is included as a compile time option (e.g. -DmyTextFileInputLists)? We could re-evaluate in the future if someone decides to support this feature.

By the way, these platform-dependent issues will be caught be AppVeyor when we move this from the developmental branch to the master. Once @fedorov responds, I think we can push a new master stable release.

[fedorov]

@fedorov would you mind if your feature request 252 is included as a compile time option (e.g. -DmyTextFileInputLists)?

No, I would not mind. The motivation for that feature was to support the use of dcm2niix as a Slicer DICOM plugin. Currently, the extension @ljod mentioned does not provide a plugin interface, but it does compile dcm2niix, so this option could be added at compile time. At some point I was planning to contribute the DICOM plugin interface to that extension.

[ljod]

This all sounds good to me thank you both!

[neurolabusc]

@ljod and @tashrifbillah - can you test the latest commit on the developmental branch with the Slicer Windows build system. The iostream getline() function looks pretty different from the stdio getline() function I was using. In the latest commit I use fgets() instead of getline(), which works in my Unix environment. It looks like fgets() is also included in (Visual Studio)[https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/fgets-fgetws?view=vs-2019] so I am cautiously optimistic.

If my latest commit does not compile with your compiler chain, remove the line:

#define myTextFileInputLists //comment out to disable this feature: https://github.com/rordenlab/dcm2niix/issues/288

and it should compile (albeit without the functionality that @fedorov requested).

[ljod]

Thank you! I think this will be automatically tested tonight by our build system. I'll give you an update here tomorrow.

[tashrifbillah]

@ljod , I think the build went fine as per the errors were concerned.

However, taking a closer look at the warnings, I am not sure why warning C4804: '>': unsafe use of type 'bool' in operation is reported. Again, warning C4146: unary minus operator applied to unsigned type, result still unsigned should not hurt.

However, warning C4700: uninitialized local variable 'dti4D' used might be a real warning having dti4D uninitialized. Although a quick fix is to initialize dti4D to an empty float, may be @neurolabusc wants to make sure the warning is legit.

[neurolabusc]

@tashrifbillah the 'uninitialized local variable' reflects that you can not overload C functions. The warning is generated by this line, where the 0 flags that the dti4D structure is not populated.
return nii_saveNRRD(niiFilename, hdr, im, opts, d, &dti4D, 0);

I added a commit that might silence some of these warnings. However, the line numbers in the warnings do not correspond with line numbers shown on Github. Feel free to submit a pull request for the other warnings.

Note that g++ and clang++ both compile without these warnings. @ningfei built the dcm2niix superbuild script and knows much more than me about MSVC. He previously noted These are all due to MSVC's poor support of the C/C++ standard. So I think it might be better that we stick with VS 2015 and don't add support for older MSVC compiler.

[ljod]

@neurolabusc thanks this is excellent! Please let us know when to switch from the development branch to the main branch.

[neurolabusc]

See new stable release (v1.0.20190410). Now that NRRD is embedded I would suggest building off the stable master branch rather than the developmental branch.

[ljod]

Thank you very much!