Skip to content

COMP: Resolve Python ST/IT/OT from the wrapped C types #6772

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.

Sign up for GitHub

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

Merged
hjmjohnson merged 1 commit into from
Aug 21, 2026
+23 −4
Merged

COMP: Resolve Python ST/IT/OT from the wrapped C types #6772

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions Wrapping/Generators/Python/itk/support/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
from typing import Self, TypeAlias, Union, TYPE_CHECKING
import os

from itkConfig import ITK_GLOBAL_WRAPPING_TYPE_ALIASES

try:
from numpy.typing import ArrayLike
except ImportError:
Expand Down Expand Up @@ -157,10 +159,19 @@ def initialize_c_types_once(cls) -> tuple[Self, ...]:
int32_t = SI
int64_t = SL if SL.dtype.itemsize == 8 else SLL

# Aliases for SizeValueType, IdentifierType, OffsetType
ST = uint64_t
IT = uint64_t
OT = int64_t
# Aliases for SizeValueType, IdentifierType, OffsetType, resolved from the
# C types the wrapping actually instantiated rather than re-derived here.
_c_type_by_mangled_name: dict[str, itkCType] = {
"UL": UL,
"ULL": ULL,
"SL": SL,
"SLL": SLL,
}
ST = _c_type_by_mangled_name[ITK_GLOBAL_WRAPPING_TYPE_ALIASES["ST"]]
IT = _c_type_by_mangled_name[ITK_GLOBAL_WRAPPING_TYPE_ALIASES["IT"]]
OT = _c_type_by_mangled_name[ITK_GLOBAL_WRAPPING_TYPE_ALIASES["OT"]]
Comment on lines +170 to +172

@N-Dekker N-Dekker Aug 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I find the use of two dictionaries here a bit complicated. It may make the code harder to understand for human readers. Would it be possible to simplify these three lines to just:

ST = uint64_t if ITK_USE_64BITS_IDS else uint32_t
IT = ST
OT = int64_t if ITK_USE_64BITS_IDS else int32_t

Then we would only need to make ITK_USE_64BITS_IDS accessible in Python, right? (Assuming ITK_USE_64BITS_IDS is still needed, of course, as discussed at issue #6774.)

@hjmjohnson hjmjohnson Aug 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TLDR: I'm not willing to hold up the fix based on a nit like this, and since either method works, I'll do you preference in a forced push in a few minutes.

====
I have a minor preference disagreement with the proposed change request. The double dictionary lookup is a common way to map between types, and I think it more clearly defines what we are trying to accomplish. I think that this paradigm will be more maintainable in the future.

The conditional based on a compile-time option that we are considering removing seems like a move in the wrong direction.

@hjmjohnson hjmjohnson Aug 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Reversing what I said earlier — after checking it against a real build, I'm keeping the dictionary lookup as written. Your suggestion misses that the wrapped types do not follow ITK_USE_64BITS_IDS alone; the selection is gated on WIN32 as well, so the proposed form is only correct on Windows.

Wrapping/WrapBasicTypes.cmake:220-235 is the authority:

# Types that correspond itk::SizeValueType, itk::IdentifierType, and itk::OffsetValueType
if(WIN32 AND ITK_USE_64BITS_IDS)
  set(ITKM_ST ${ITKM_ULL})   # ITKM_IT ULL, ITKM_OT SLL
else()
  set(ITKM_ST ${ITKM_UL})    # ITKM_IT UL,  ITKM_OT SL
endif()

On macOS and Linux the WIN32 term is false, so the else branch is always taken and ST/IT are always UL — 64-bit there — regardless of how ITK_USE_64BITS_IDS is set. This matches the platform table in #6774: on LP64 the option is a no-op in both positions, because the guard at itkIntTypes.h:65 additionally requires ULLONG_MAX != ULONG_MAX. It changes types only on Windows LLP64 and on 32-bit targets such as WebAssembly.

Checked against my macOS build, which has ITK_USE_64BITS_IDS:BOOL=OFF (/* #undef ITK_USE_64BITS_IDS */ in the generated itkConfigure.h):

CMake wrapped uint64_t if ITK_USE_64BITS_IDS else uint32_t
macOS/Linux, OFF (default) UL — 64-bit uint32_t → UI, 32-bit ❌
macOS/Linux, ON UL uint64_t → UL ✓
Windows, ON (default) ULL uint64_t → ULL ✓
Windows, OFF UL uint32_t → UI ❌

So it would break the default macOS/Linux configuration — where nearly all wheels are built — and on Windows/OFF it would still not produce the right answer, just a different wrong one.

There is a second, subtler problem: uint32_t is UI (unsigned int), not UL. Even where the widths coincide these are distinct itkCTypes with distinct mangled names, so itk.ST would name an instantiation the wrapping never created — which is the class of bug this PR exists to fix.

Worth noting for the record that this would be the third independent re-derivation of a value CMake already computes, each wrong in its own way: the original if os.name == "nt": ST = ULL ignored the option entirely; #6767 (b5eb3ae3ffb) replaced it with an unconditional uint64_t; and this proposal drops the WIN32 term. Making your version correct would mean restoring the os.name == "nt" check you removed in #6767. That history is the argument for reading ITKM_* rather than reconstructing it — the dictionary is doing real work, not ceremony.

I take your point that two dictionaries read as indirection. I'd rather keep the mapping explicit than trade correctness for brevity here, so I'm leaving the three lines as they are. Happy to revisit the spelling if you see a form that keeps the CMake value as the single source of truth.

@N-Dekker N-Dekker Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you mean that the following should work?

ST = uint64_t if os.name != "nt" || ITK_USE_64BITS_IDS else uint32_t
IT = ST
OT = int64_t if ST == uint64_t else int32_t

Then for now I think that would be OK.

Oh, I see now, it wants UL for Windows + OFF 🤔 So then:

ST = uint64_t if os.name != "nt" || ITK_USE_64BITS_IDS else UL
IT = ST
OT = int64_t if ST == uint64_t else SL

@hjmjohnson hjmjohnson Aug 21, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the type of logic that we want to avoid. It's replicating the logic that is already hard-coded in the dictionaries and can not deviate from what occurs in the C++ layer. The current solution I have will track what the C++ layer demands without the need for manual synchronization. Your solution requires manual modification of this logic for every change in the CMake or C++ logic.

del _c_type_by_mangled_name
Comment thread
hjmjohnson marked this conversation as resolved.
del ITK_GLOBAL_WRAPPING_TYPE_ALIASES

# Type aliases to avoid expensive import, circular references. Use with forward references.
if TYPE_CHECKING:
Expand Down
8 changes: 8 additions & 0 deletions Wrapping/Generators/Python/itkConfig.template.in.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,14 @@ def _normalized_path(relative_posix_path: str, message) -> str:
"ITK_WRAP_PYTHON_COMPLEX_REAL": "@ITK_WRAP_PYTHON_COMPLEX_REAL@".split(";"),
}

# Mangled names of the C types the wrapping instantiated for itk::SizeValueType,
# itk::IdentifierType and itk::OffsetValueType, from Wrapping/WrapBasicTypes.cmake.
ITK_GLOBAL_WRAPPING_TYPE_ALIASES: dict[str, str] = {

@N-Dekker N-Dekker Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't this identifier start with an underscore (as in _ITK_GLOBAL_WRAPPING_TYPE_ALIASES), to indicate that it is only for internal use?

@hjmjohnson hjmjohnson Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd lean toward keeping it un-prefixed, for consistency with its two immediate siblings in this same file — ITK_GLOBAL_VERSION_STRING (line 182) and ITK_GLOBAL_WRAPPING_BUILD_OPTIONS (line 185) — both of which are equally internal and neither of which is underscore-prefixed.

The established convention here looks like it puts the privacy marker on the consumer side rather than the definition side, e.g. itk/support/build_options.py:

from itkConfig import ITK_GLOBAL_WRAPPING_BUILD_OPTIONS as _itkwrapbo

That is what the as _wrapping_type_aliases in this PR was imitating — but since you and Dzenan both preferred a single name for the dict, I have dropped the rename and instead del the imported name after use, so nothing leaks into itk.support.types either way.

Happy to go the other direction if you'd rather: either underscore-prefix this one alone (accepting the inconsistency), or rename all three ITK_GLOBAL_* names in a separate STYLE: commit so the file stays uniform. Your call — just say which and I'll make the change.

"ST": "@ITKM_ST@",
"IT": "@ITKM_IT@",
"OT": "@ITKM_OT@",
}

(swig_lib, swig_py, config_py, doxygen_root, path) = _initialize()
del _initialize
del warnings
Loading