Skip to content

ENH: Increase pixel type alias consistency in common operators#2550

Merged
dzenanz merged 2 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreasePixelTypeAliasConsistencyInCommonOperators
May 27, 2021
Merged

ENH: Increase pixel type alias consistency in common operators#2550
dzenanz merged 2 commits intoInsightSoftwareConsortium:masterfrom
jhlegarreta:IncreasePixelTypeAliasConsistencyInCommonOperators

Conversation

@jhlegarreta
Copy link
Copy Markdown
Member

Increase pixel type alias consistency in common operators:

  • Make PixelType alias be public in itk::AnnulusOperator
  • Prefer aliasing template pixel type in common operator classes

PR Checklist

Make `PixelType` alias be `public` in `itk::AnnulusOperator`.

This will allow using the appropriate types for the `m_InteriorValue`,
`m_AnnulusValue`, and `m_ExteriorValue` members for an instance of the
class.
Prefer aliasing template pixel type in `common` operator classes over
aliasing the superclass pixel type, which is earlier set to the template
pixel type.
@github-actions github-actions Bot added area:Core Issues affecting the Core module type:Enhancement Improvement of existing methods or implementation labels May 20, 2021
Copy link
Copy Markdown
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

Is the use of of typename Superclass::PixelType in the replaced code supposed to be a way to tell the programmer that these classes inherit their type aliases from the superclass ... kind of paralleling how these classes inherit methods from the superclass?

Does the ITK style guide say anything about this? If not, should it?

@jhlegarreta
Copy link
Copy Markdown
Member Author

@Leengit Not sure about the intention of using the Superclass type for the pixel, although it is exactly the same as the current classes'. Maybe it was more consistent with the rest of the aliases. I'd say that the SW Guide does not propose or encourage a practice here. If you feel like it should, please do a PR. If you think that the former solution should be used, I'd happy to delete the relevant commit in this PR.

Copy link
Copy Markdown
Contributor

@Leengit Leengit left a comment

Choose a reason for hiding this comment

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

I see value in both the typename Superclass::PixelType and TPixel approaches. I have a slight preference for the former, but nothing significant. Either way ... looks good to me.

@dzenanz dzenanz merged commit 9f31b6a into InsightSoftwareConsortium:master May 27, 2021
@jhlegarreta jhlegarreta deleted the IncreasePixelTypeAliasConsistencyInCommonOperators branch May 27, 2021 16:15
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 type:Enhancement Improvement of existing methods or implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants