Catching C++ itkAssertOrThrowMacro in Python

[Leengit]

Description

This happens generally in ITK, but a specific example is: the itk-spcn / StructurePreservingColorNormalzationFilter cannot handle all input images and the C++ code currently throws via an itkAssertOrThrowMacro when it discovers that it cannot handle an input image (e.g., because the RGB image is nonetheless too monochromatic). The C++ throw includes an informative message. However, this triggers a segmentation fault when used in Python (after pip install itk-spcn and import itk) and does not relay the informative message to the user.

This should be changed so that the Python user of this code gets the informative error message.

Version

ITK sources as of May 27, 2022.

Environment

Ubuntu. Perhaps others.

[Leengit]

On discourse, @thewtex writes:

Can we update the itkAssertOrThrowMacro to throw if ITK_WRAPPING is defined? Throwing an exception should result in a traceback / Python erorr in Python instead of a program crash.

[N-Dekker]

@Leengit @thewtex Just looking at this issue again, hope you still remember 😃

Looking at itkAssertInDebugAndIgnoreInReleaseMacro, I wonder if it cannot be simplified! Here it is now:

https://github.com/InsightSoftwareConsortium/ITK/blob/4543ba6dce61bdc4540bb0641b2aafae10056b9a/Modules/Core/Common/include/itkMacro.h#L790-L794

#if !defined(NDEBUG) && !defined(ITK_WRAPPING)
#  define itkAssertInDebugAndIgnoreInReleaseMacro(X) assert(X)
#else
#  define itkAssertInDebugAndIgnoreInReleaseMacro(X) ITK_NOOP_STATEMENT
#endif

As you probably know, the standard way to disable assert on the entire project is by defining NDEBUG, of course. Quoting https://en.cppreference.com/w/c/error/assert:

If NDEBUG is defined ... then assert does nothing.

So when building Python wrappings, shouldn't NDEBUG always be defined on the entire build? (For example, by CMake add_compile_definitions(NDEBUG). Or by selecting build configuration Release, MinSizeRel, or RelWithDebInfo.)

If so, can't the macro simply be defined unconditionally (without an #if) as:

#define itkAssertInDebugAndIgnoreInReleaseMacro(X) assert(X)

?

[Leengit]

That would be consistent with the name itkAssertInDebugAndIgnoreInReleaseMacro. As you say, because we use the macro to mean "assert except if in release mode or if wrapping itk" this all depends upon whether it is the case that we want defined(ITK_WRAPPING) to always set defined(NDEBUG). Do we want to enforce that, or is there some consequence of !defined(NDEBUG) other than treatment of assert statements that the user might want even when defined(ITK_WRAPPING)?

[N-Dekker]

"ITK\Modules\ThirdParty" has more than 1000 C/C++ assert statements, so if you want to avoid any assert failure when building the Python wrappings, I think you should indeed make sure that NDEBUG is defined for the entire project.