ImageIOBase: Simplify GetComponentSize(), deprecate SetTypeInfo, rename TPixel to TComponent#5438
Merged
thewtex merged 3 commits intoInsightSoftwareConsortium:mainfrom Jun 24, 2025
Conversation
Replaced an exhaustive (boilerplate) `switch` statement. Made `ComponentTypeTraits::sizeOfComponent` an `unsigned int`, as that is also the return type of GetComponentSize().
`SetTypeInfo(const TPixel *)` was introduced with commit 383216f, 11 Nov 2010, but it was never implemented and it was not specialized.
When a template parameter specifies a pixel _component_ type, rather than a pixel type, it appears clearer to name it `TComponent` (similar to `itk::RGBPixel<TComponent>` in "itkRGBPixel.h"). Adjusted the documentation of `MapPixelType` accordingly.
There was a problem hiding this comment.
Pull Request Overview
This PR streamlines component size retrieval, removes an unused legacy method, and aligns template parameter naming with upstream conventions.
- Simplify
GetComponentSize()by delegating toGetComponentTypeTraits - Deprecate unimplemented
SetTypeInfo(const TPixel*)viaitkLegacyMacro - Rename template parameter
TPixeltoTComponentand adjust trait member type
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| Modules/IO/ImageBase/src/itkImageIOBase.cxx | Simplified GetComponentSize() implementation using type traits |
| Modules/IO/ImageBase/include/itkImageIOBase.h | Deprecated legacy SetTypeInfo, updated GetNumberOfBitsOfComponentType, renamed TPixel to TComponent, and changed sizeOfComponent to unsigned int |
Comments suppressed due to low confidence (3)
Modules/IO/ImageBase/src/itkImageIOBase.cxx:357
- Add unit tests for each
IOComponentEnumvalue—especiallyUNKNOWNCOMPONENTTYPE—to verify thatGetComponentSizereturns correct sizes and throws on unknown types.
ImageIOBase::GetComponentSize() const
Modules/IO/ImageBase/src/itkImageIOBase.cxx:359
- [nitpick] Use
autoforsizeOfComponentto decouple fromunsigned int, e.g.,if (const auto sizeOfComponent = ...so that changes inGetComponentTypeTraitsdon't require updating this type.
if (const unsigned int sizeOfComponent = GetComponentTypeTraits(m_ComponentType).sizeOfComponent; sizeOfComponent > 0)
Modules/IO/ImageBase/include/itkImageIOBase.h:967
- [nitpick] Consider using
size_tforsizeOfComponentto match platform-dependent sizes and avoid potential narrowing if component sizes ever exceedUINT_MAX.
unsigned int sizeOfComponent;
dzenanz
approved these changes
Jun 24, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three style commits:
GetComponentSize()callGetComponentTypeTraits, instead of having a boilerplateswitchSetTypeInfo(const TPixel *), which was introduced by commit 383216f (11 Nov 2010), and appears unimplemented and unusedTPixeltoTComponent, just likeitk::RGBPixel<TComponent>:ITK/Modules/Core/Common/include/itkRGBPixel.h
Lines 57 to 58 in 50a5d3c