WIP: [Do not merge] Change the default char type to unsigned

[N-Dekker]

Just as an experiment!

Triggered by @blowekamp at #5450 (comment)

[N-Dekker]

itkHDF5ImageIOTest fails because of a bug (missing unsigned char support) in itkHDF5ImageIO.cxx, I think. Related to this code:

ITK/Modules/IO/HDF5/src/itkHDF5ImageIO.cxx

Lines 783 to 787 in 8de372e

	else if (metaDataType == H5::PredType::NATIVE_CHAR)
	{
	this->StoreMetaData<char>(&metaDict, localMetaDataName, name, metaDataDims[0]);
	}
	else if (metaDataType == H5::PredType::NATIVE_UCHAR)

So it treats NATIVE_CHAR different from NATIVE_UCHAR, even when the native (default) char is unsigned (equivalent to unsigned char).

However, I must say, I don't understand exactly what's going on there 🤷

[dzenanz]

Should this be closed or rebased, given that #5473 was merged?

[N-Dekker]

Should this be closed or rebased, given that #5473 was merged?

Thanks for asking, @dzenanz. It's still useful for me to keep this PR open for a little while, although it is never meant to be merged. I believe there are still a few small char/signed char issues in the source tree, which can be revealed by testing ITK with /J (MSVC) or -funsigned-char (GCC), as this PR does.

[dzenanz]

Should second and third commit be put into a separate PR, meant to be merged?

[N-Dekker]

Should second and third commit be put into a separate PR, meant to be merged?

• OK, for VTKPolyDataMeshIO: pull request BUG: VTKPolyDataMeshIO should use signed char for the SCHAR cases #5548

The HDF5ImageIO commit ("ENH: Support H5::PredType::NATIVE_SCHAR...") might be OK as well, but as I still see a failure of itkHDF5ImageIOTest, it may not be good enough yet 🤷

[hjmjohnson]

@N-Dekker I think the content of this PR is superceeded by changes needed to make linxu-arm build work

• ENH: Add Linux ARM in CI and fix char is 'unsigned' bugs #5137

I am closing this PR (reopen if you think this has independant value)

[N-Dekker]

@hjmjohnson This PR still has test failures, indicating that we still assume that char is a signed integer type, erroneously. So I think it's still useful to have this PR. (Even though it isn't meant to be merged.)

[hjmjohnson]

@N-Dekker The test failures in this PR are fixed in the recent MINC updates, and in preparation for Linux-arm CI testing.

• Fix usage of CHAR_MIN and CHAR_MAX BIC-MNI/libminc#134
• ENH: Add Linux ARM in CI and fix char is 'unsigned' bugs #5137

I used your CMakeList.txt code use compiler flags that force char to be unsigned in debugging the Linux-arm failures.

If this PR is rebased on top of

• ENH: Add Linux ARM in CI and fix char is 'unsigned' bugs #5137

I think it will start passing, and only the "DO NOT SUBMIT" patch will remain.