Skip to content

STYLE: Replace itk::NumericTraits<unsigned int>::OneValue() with 1#4467

Merged
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Replace-OneValue-with-1
Feb 16, 2024
Merged

STYLE: Replace itk::NumericTraits<unsigned int>::OneValue() with 1#4467
hjmjohnson merged 1 commit intoInsightSoftwareConsortium:masterfrom
N-Dekker:Replace-OneValue-with-1

Conversation

@N-Dekker
Copy link
Copy Markdown
Contributor

Follow-up to pull request #4460 commit 5d746cc
"STYLE: Replace NumericTraits<unsigned int>::ZeroValue() with 0U"

Follow-up to pull request InsightSoftwareConsortium#4460
commit 5d746cc
"STYLE: Replace `NumericTraits<unsigned int>::ZeroValue()` with `0U`"
@github-actions github-actions bot added area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming) labels Feb 16, 2024
Copy link
Copy Markdown
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Does 1 vs 1u make any difference?

@N-Dekker
Copy link
Copy Markdown
Contributor Author

Does 1 vs 1u make any difference?

In practice, when you assign the value (1 or 1u) to a variable of a built-in type, it does not make a difference at all. So it's just a matter of style 😃

@blowekamp
Copy link
Copy Markdown
Member

I am pretty sure at some point there was a compiler warning flag used that warned about conversions from int to unsigned int, and the draconian usage itk::NumericTraits was used to help determine where real conversion issues were occurring.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 16, 2024

itkOBJMeshIO.cxx does m_NumberOfCellPixelComponents = 3 without u as well.

this->m_PointDimension = 3;

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 16, 2024

If anyone insists on 1U, that's OK to me as well, I can amend the commit if anyone wants to. I'm just slightly in favor of 1. Because of its simplicity 😃

BTW, I would use uppercase U (rather than lower case) in order to be consistent with the L for long. Or UL for unsigned long.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

N-Dekker commented Feb 16, 2024

I am pretty sure at some point there was a compiler warning flag used that warned about conversions from int to unsigned int, and the draconian usage itk::NumericTraits was used to help determine where real conversion issues were occurring.

Thanks for explaining, @blowekamp. Nowadays I think the main reason for using itk::NumericTraits would be to support pixel types in a generic way. Specifically, itk::NumericTraits<PixelType>::OneValue() returns 1 when the pixel type is int, while it returns {1, 1, 1} for an RGB pixel type.

@blowekamp
Copy link
Copy Markdown
Member

I am pretty sure at some point there was a compiler warning flag used that warned about conversions from int to unsigned int, and the draconian usage itk::NumericTraits was used to help determine where real conversion issues were occurring.

Thanks for explaining, @blowekamp. Nowadays I think the main reason for using itk::NumericTraits would be to support pixel types in a generic way. Specifically, itk::NumericTraits<PixelType>::OneValue() returns 1 when the pixel type is int, while it returns {1, 1, 1} for an RGB pixel type.

The other case to consider is with itk::SizeType or std::std size type. The size of "long" and "long long" vary across systems and can change based on the word size for the architecture on some, but not other platforms.

@N-Dekker
Copy link
Copy Markdown
Contributor Author

Nowadays I think the main reason for using itk::NumericTraits would be to support pixel types in a generic way. Specifically, itk::NumericTraits<PixelType>::OneValue() returns 1 when the pixel type is int, while it returns {1, 1, 1} for an RGB pixel type.

The other case to consider is with itk::SizeType or std::std size type. The size of "long" and "long long" vary across systems and can change based on the word size for the architecture on some, but not other platforms.

OK, but for a built-in integer type (or a typedef or alias of a built-in integer type), std::numeric_limits<T> typically offers everything one may need. So in that case, I would prefer std::numeric_limits<T>, rather than itk::NumericTraits<T>.

@N-Dekker N-Dekker marked this pull request as ready for review February 16, 2024 18:04
@hjmjohnson hjmjohnson merged commit 63b0c85 into InsightSoftwareConsortium:master Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants