Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

COMP: Add type aliases of ptrdiff_t and size_t to the itk namespace #4622

Merged

Conversation

N-Dekker
Copy link
Contributor

Officially, we may not assume that ptrdiff_t and size_t are defined in the global namespace.


Motivation: Both ptrdiff_t and size_t are already commonly being used inside the itk namespace, without specifying their std namespace:

Convert(const InputPixelType * inputData, int inputNumberOfComponents, OutputPixelType * outputData, size_t size);

It just compiles, so far! However, compilers may become more picky, as those types are not guaranteed to remain available in the global namespace.

Officially, we may not assume that `ptrdiff_t` and `size_t` are defined in the
global namespace.
@github-actions github-actions bot added type:Compiler Compiler support or related warnings area:Core Issues affecting the Core module labels Apr 29, 2024
Copy link
Member

@hjmjohnson hjmjohnson left a comment

Choose a reason for hiding this comment

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

This seems like a good solution to being backward compatible.

@N-Dekker N-Dekker marked this pull request as ready for review April 30, 2024 08:29
@N-Dekker
Copy link
Contributor Author

@thewtex FYI, I think this pull request can be merged safely for ITK 5.4. On the other hand, it isn't very urgent to me, personally. It might become urgent, once compilers start to complain about using size_t without specifying the namespace std.

@dzenanz
Copy link
Member

dzenanz commented Apr 30, 2024

This looks like a low-risk addition, and merging it sooner rather than later is better in my opinion.

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

@N-Dekker thanks for the patch 👍 This does seem to be low risk; however, it is also possible to cause issues since there may be naming conflicts. Since this is not addressing a known issue, it should be merged after v5.4.0.

@N-Dekker N-Dekker marked this pull request as draft May 1, 2024 17:49
@N-Dekker
Copy link
Contributor Author

N-Dekker commented May 1, 2024

This does seem to be low risk; however, it is also possible to cause issues since there may be naming conflicts. Since this is not addressing a known issue, it should be merged after v5.4.0.

@thewtex OK, no problem. Marked "draft"to avoid accidentally being merged before the release of v5.4.0. 👍

@N-Dekker N-Dekker marked this pull request as ready for review May 21, 2024 08:08
@N-Dekker
Copy link
Contributor Author

I think this pull request may also be merged now, as v5.4.0 has been tagged: #4603 (comment)

@thewtex thewtex merged commit 163984c into InsightSoftwareConsortium:master May 21, 2024
14 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 type:Compiler Compiler support or related warnings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants