Skip to content

Conversation

@N-Dekker
Copy link
Contributor

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

  • Added a note about IOComponent::CHAR representing signed char
  • Let file readers interpret the CHAR enum as signed char (instead of just plain char)
  • Adjusted tests accordingly

Triggered by a discussion with Bradley (@blowekamp) at https://discourse.itk.org/t/long-double-support-for-imageio-meshio-pixels/7570/11 and his comment at #5449 (comment)

The signedness of the C++ type `char` depends on platform and compilation, so it
is preferable to use `signed char` for signed numeric pixel values.
@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module area:IO Issues affecting the IO module labels Jul 3, 2025
@N-Dekker N-Dekker changed the title ENH: Let IOComponent::CHAR represents signed char Let IOComponent::CHAR represents signed char Jul 3, 2025
N-Dekker added 3 commits July 3, 2025 22:19
Let ImageFileReader, MeshFileReader, and VideoFileReader interpret
`IOComponentEnum::CHAR` as `signed char`, instead of `char`. This ensures
that 8-bits signed data files are properly interpreted.
Let both HashTestImage (itkTestDriverInclude) and itkTIFFImageIOCompressionTest
interpret `IOComponent::CHAR` as `signed char`
Let `ImageIOBase::ReadBufferAsASCII` interpret `IOComponentEnum::CHAR` as
`signed char`
@N-Dekker N-Dekker force-pushed the IOComponent-CHAR-signed-char branch from 3159a22 to d26cce4 Compare July 3, 2025 20:27
@github-actions github-actions bot added type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Video Issues affecting the Video module and removed type:Enhancement Improvement of existing methods or implementation labels Jul 3, 2025
@N-Dekker N-Dekker marked this pull request as ready for review July 4, 2025 09:36
@N-Dekker N-Dekker requested a review from blowekamp July 4, 2025 09:37
Copy link
Member

@blowekamp blowekamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this change. This change now matches how I have seen this enum type used for ImageIO. Getting this cleanup up and documented is a good step forward.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Jul 8, 2025

For the record, signed char was already mapped to IOComponentEnum::CHAR before, by calling IMAGEIOBASE_TYPEMAP(signed char, IOComponentEnum::CHAR), at:

IMAGEIOBASE_TYPEMAP(signed char, IOComponentEnum::CHAR);

This logic was syntactically restyled (but not altered, sematically) into std::is_same_v<TComponent, signed char> ? IOComponentEnum::CHAR by pull request #5438, at:

: std::is_same_v<TComponent, signed char> ? IOComponentEnum::CHAR

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Jul 8, 2025

As far as I can see, signed char and std::int8_t denote the very same type, on all supported platforms:

static_assert(std::is_same_v<std::int8_t, signed char>);

On the other hand, signed char and char are always distinct types, by definition, even when char is a signed type:

static_assert(! std::is_same_v<signed char, char>);

https://godbolt.org/z/3dGcx4vdP

@blowekamp
Copy link
Member

I just saw that GCC has the -funsigned-char and -fsigned-char it might be an interesting experiment to see if ITK works with both states. I am not sure if any of the support platform as char as signed.

@N-Dekker
Copy link
Contributor Author

N-Dekker commented Jul 9, 2025

I just saw that GCC has the -funsigned-char and -fsigned-char it might be an interesting experiment to see if ITK works with both states.

@blowekamp The test failures of my experiment show that ITK currently does not work with -funsigned-char (letting the default char be an unsigned type)

The failures can typically be fixed by explicitly using signed char, instead of just plain char. So this pull request (#5450) is a step in the right direction, I think 😇

@hjmjohnson hjmjohnson merged commit 4d45bad into InsightSoftwareConsortium:main Jul 10, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Core Issues affecting the Core module area:IO Issues affecting the IO module area:Video Issues affecting the Video module type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants