STYLE: Use itk::Math::Absolute() instead of abs to communicate subtle implementation differences#5802
STYLE: Use itk::Math::Absolute() instead of abs to communicate subtle implementation differences#5802hjmjohnson wants to merge 4 commits intomainfrom
Conversation
github-actions
bot
added
the
type:Style
hjmjohnson
force-pushed
the
instrument-safe_abs-names
branch
from
d754a92 to
7a09873
Compare
hjmjohnson
force-pushed
the
instrument-safe_abs-names
branch
from
7a09873 to
189ab05
Compare
hjmjohnson
changed the title
hjmjohnson
force-pushed
the
instrument-safe_abs-names
branch
from
189ab05 to
b4a79d8
Compare
- Removed redundant definitions of `IsPrime` and `GreatestPrimeFactor` in `itkMath.cxx` by incorporating template-based implementations in `itkMath.h`. - Improved compile-time evaluation and type safety with `constexpr` and `std::enable_if_t`. - Updated tests to leverage the new `constexpr` `itk::Math` functions. - Enhanced maintainability by consolidating code and leveraging modern C++ features.
Use GTest framework for testing the math functions.
- Updated the function name from `safe_abs` to `Absolute` in itkMath.h for clarity and consistency with ITK naming conventions. - Revised function comments to reflect the name change and provide additional details about differences with `std::abs`. - Modified corresponding test cases in itkMathGTest.cxx to validate the renamed `Absolute` function, ensuring accurate behavior and backward compatibility.
The itk::Math::Absolute is subtly different from the std::abs and other typical absolute value functions, so use the more explicit name to clearly indicate that the behaviors are not the same as the core c++ similarly named functions. Use ITK_FUTURE_LEGACY_REMOVE of the itk::Math::abs supported name.
hjmjohnson
force-pushed
the
instrument-safe_abs-names
branch
from
b4a79d8 to
d36ba88
Compare
|
@hjmjohnson Very nice improvements! Thank you for following my In general, please try to keep pull requests a bit smaller. 🙏 As a reviewer, I find PRs of more than a thousand lines of code a bit overwhelming. (In fact, I usually cannot review PRs of 1000+ lines very carefully anymore.) A commit like "Convert itkMathTest to itkMathGTest" may be a candidate for a separate PR. |
:). That was why I kept this as a Draft and have Depends on : add constexpr for math operations #5798 in the PR message. Thanks for reviewing this. Once #5798 is reviewed and approved, Ill rebase and remove Draft status. |
2 participants
Depends on :
PR Checklist