InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.4k stars 662 forks source link

STYLE: Introduce constants for the default tolerances #4483

Closed blowekamp closed 5 months ago

blowekamp commented 6 months ago

Consolidates constants used for default tolerances into the itkMacro.h header file.

PR Checklist

Refer to the ITK Software Guide for further development details if necessary.

N-Dekker commented 6 months ago

Are the constants you are proposing here meant to be part of the public API? Just wondering. Personally I think it's fine to have them in the public API. In that case, you may even call it an enhancement (ENH), rather than just a style improvement (STYLE). 😃

N-Dekker commented 6 months ago

Looks like the ITK.Windows CI failure (itkConvolutionImageFilterStreamingTest) is unrelated: https://open.cdash.org/tests/1443991348 But I don't really know 🤷

blowekamp commented 6 months ago

Regarding the name, I struggled to find other example of global constants in ITK, so I was not sure what naming convention to use. The closest I could find what itkCommonEnum, but they are just enum and not constant real numbers.

N-Dekker commented 5 months ago

I think you need the C++17 inline constexpr if it is used in a header.

Interesting, thanks @hjmjohnson 😃 Honesty I don't have experience with inline constexpr, do you? I do believe, in practice the original const works fine for global variables, also in a header. As initially proposed by Bradley:

const double itkDefaultCoordinateTolerance = 1e-6;

With const, different cxx files might see a different itkDefaultCoordinateTolerance instance. At a different address, &itkDefaultCoordinateTolerance. But they would all have the same value anyway, 1e-6.

With inline constexpr I assume that all cxx files would share one and the same itkDefaultCoordinateTolerance instance, at the same address, &itkDefaultCoordinateTolerance. This might certainly be a nice property. But I just don't know if it's necessary 🤷

blowekamp commented 5 months ago

The inline specifier looks like a good hint for the compiler.

There appears to be concusses to move these definitions to the itkImageBase header. I assume in the itk namespace and not as a constant member variable.

I am still looking for what the names should be. Maybe ImageDefaultCoordinateTolerance?

N-Dekker commented 5 months ago

The inline specifier looks like a good hint for the compiler.

I guess the inline specifier might be slightly more work for the compiler, because it then has to make sure that the constant has one unique instance (at a unique address). But as long as it works, it's OK to me.

I am still looking for what the names should be. Maybe ImageDefaultCoordinateTolerance?

Personally I would put the word "Default" at the begin (as in "DefaultImageCoordinateTolerance"), but I guess that's just a matter of taste! Either way is OK to me.