Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

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

wants to merge 3 commits into from

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Jul 8, 2025

Just as an experiment!

Triggered by @blowekamp at #5450 (comment)

@github-actions github-actions bot added type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots area:IO Issues affecting the IO module labels Jul 8, 2025
@N-Dekker

This comment was marked as outdated.

Sign in to view
@N-Dekker N-Dekker mentioned this pull request Jul 9, 2025
Merged
@N-Dekker
Copy link
Contributor Author

N-Dekker commented Jul 9, 2025

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 🤷

@N-Dekker N-Dekker force-pushed the Support-char-as-unsigned-type branch from 6852856 to 724f51a Compare July 10, 2025 15:56
@github-actions github-actions bot added area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module labels Jul 10, 2025
@N-Dekker N-Dekker force-pushed the Support-char-as-unsigned-type branch 2 times, most recently from 2bb4eac to 4b63bf3 Compare July 10, 2025 21:27
@N-Dekker N-Dekker mentioned this pull request Jul 11, 2025
Merged
@N-Dekker N-Dekker force-pushed the Support-char-as-unsigned-type branch from 4b63bf3 to 4604362 Compare July 13, 2025 21:01
@github-actions github-actions bot removed area:Python wrapping Python bindings for a class type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module area:Filtering Issues affecting the Filtering module area:Registration Issues affecting the Registration module area:Segmentation Issues affecting the Segmentation module labels Jul 13, 2025
@N-Dekker N-Dekker force-pushed the Support-char-as-unsigned-type branch 2 times, most recently from b8ab121 to c163521 Compare July 17, 2025 14:13
@dzenanz
Copy link
Member

dzenanz commented Jul 25, 2025

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

@N-Dekker N-Dekker force-pushed the Support-char-as-unsigned-type branch from c163521 to 0c97bdf Compare July 25, 2025 20:37
@N-Dekker
Copy link
Contributor Author

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.

@N-Dekker
WIP: [Do not merge] Change the default char type to unsigned
ab85cc0 
Just as an experiment!
@N-Dekker
ENH: Support H5::PredType::NATIVE_SCHAR for signed char
7892d7c 
Mapped `H5::PredType::NATIVE_CHAR` to either `IOComponentEnum::SCHAR` or
`IOComponentEnum::UCHAR`, depending on the signedness of `char`.
@N-Dekker
BUG: VTKPolyDataMeshIO should use signed char for the CHAR cases
18e206e 
`VTKPolyDataMeshIO` originally casted the `buffer` to plain `char *` for
component type `CHAR`. However, `CHAR` indicates that the component type is
_signed_ char, whereas the default `char` might be unsigned.

Follow-up to pull request InsightSoftwareConsortium#5470
commit 70fbb2b "BUG: MINCImageIO should use
`signed char` for `CHAR` and `MI_TYPE_BYTE`"
@N-Dekker N-Dekker force-pushed the Support-char-as-unsigned-type branch from 0c97bdf to 18e206e Compare August 27, 2025 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area:IO Issues affecting the IO module type:Infrastructure Infrastructure/ecosystem related changes, such as CMake or buildbots
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@N-Dekker @dzenanz