Make the index type of VectorContainer itk::SizeValueType by default
#4856
Conversation
This file contains 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
github-actions
bot
added
type:Testing
N-Dekker
commented
Sep 20, 2024
N-Dekker
force-pushed
the
VectorContainer-default-index
branch
2 times, most recently
from
9269414
to
114e2b5
Compare
dzenanz
approved these changes
Sep 27, 2024
N-Dekker
commented
Sep 27, 2024
Comment on lines
30
to
31
| namespace ClassTemplate | ||
| { |
There was a problem hiding this comment.
This pull request places the original class template VectorContainer in a nested namespace, itk::ClassTemplate, so that it will still be available to users who really need to access the original class. Usually, users may just use itk::VectorContainer (as before), which will then be an alias to itk::ClassTemplate::VectorContainer.
This approach is a bit of a workaround. Maybe we can eventually just define itk::VectorContainer<TElement> as a template class, without the TElementIdentifier parameter. But that would be a breaking change. 🤷
There was a problem hiding this comment.
I think we should use itk::detail:: here instead of creating a new namespace.
There was a problem hiding this comment.
Thanks for the suggestion @thewtex. Addressed by my latest force-push, please check!
thewtex
requested changes
Oct 2, 2024
| */ | ||
| template <typename T1, typename T2 = void> | ||
| using VectorContainer = ClassTemplate::VectorContainer<std::conditional_t<std::is_void_v<T2>, SizeValueType, T1>, | ||
| std::conditional_t<std::is_void_v<T2>, T1, T2>>; |
Comment on lines
30
to
31
| namespace ClassTemplate | ||
| { |
There was a problem hiding this comment.
I think we should use itk::detail:: here instead of creating a new namespace.
By default, assume `ElementIdentifier = SizeValueType`, for an
`itk::VectorContainer<ElementType>`.
Conceptually like:
template <typename TElementIdentifier = SizeValueType, typename TElement>
class VectorContainer;
Implemented by moving the old `VectorContainer` definition into the
`itk::detail` namespace, and introducing a new
`itk::VectorContainer<T1, T2>` alias template.
The index type (`ElementIdentifier`) of `itk::VectorContainer` is already `itk::SizeValueType` by default.
Also removed an `IdentifierType` alias. The index type (`ElementIdentifier`) of `itk::VectorContainer` is already `itk::SizeValueType` by default, and `itk::IdentifierType` is just an alias of `itk::SizeValueType`.
N-Dekker
force-pushed
the
VectorContainer-default-index
branch
from
114e2b5
to
edae0bc
Compare
dzenanz
approved these changes
Oct 3, 2024
N-Dekker
added a commit
to N-Dekker/ITK
that referenced
this pull request
Oct 10, 2024
Following pull request InsightSoftwareConsortium#4856 commit edae0bc "STYLE: Remove IdentifierType template argument from VectorContainer uses"
N-Dekker
added a commit
to N-Dekker/ITK
that referenced
this pull request
Oct 10, 2024
Simplified the tests by just using the default element identifier type, which is `itk::SizeValueType`. Following pull request InsightSoftwareConsortium#4856 commit edae0bc "STYLE: Remove IdentifierType template argument from VectorContainer uses"
N-Dekker
added a commit
to N-Dekker/ITK
that referenced
this pull request
Oct 10, 2024
Following pull request InsightSoftwareConsortium#4856 commit edae0bc "STYLE: Remove IdentifierType template argument from VectorContainer uses"
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.
For most use cases, it appears that the index type of
itk::VectorContainershould just beitk::SizeValueType(oritk::IdentifierType, but that's just an alias ofitk::SizeValueType).This pull request allows users to write
itk::VectorContainer<ElementType>as a shorthand foritk::VectorContainer<itk::SizeValueType, ElementType>.