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

ENH: Use a macro to print numeric traits values of objects #3909

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jhlegarreta
Copy link
Member

@jhlegarreta jhlegarreta commented Feb 4, 2023

Use a macro to print numeric traits values of objects. Avoids boilerplate code.

Printed instances found using the regex:

os << indent << "(.*): " << static_cast<typename NumericTraits<(.*)>::PrintType>(m_(.*)) << std::endl;

and substituted using itkPrintSelfNumericTraitsMacro($3, $2);.

PR Checklist

@github-actions github-actions bot added type:Enhancement Improvement of existing methods or implementation area:Core Issues affecting the Core module labels Feb 4, 2023
@jhlegarreta jhlegarreta force-pushed the UseAMacroToPrintNumericTraitsValuesOfObjects branch from ee3e1f5 to 8deb9fc Compare February 4, 2023 17:10
Use a macro to print numeric traits values of objects. Avoids
boilerplate code.

Printed instances found using the regex:
```
os << indent << "(.*): " << static_cast<typename NumericTraits<(.*)>::PrintType>(m_(.*)) << std::endl;
```

and substituted using itkPrintSelfNumericTraitsMacro\($3\, \$2\);.
@jhlegarreta jhlegarreta force-pushed the UseAMacroToPrintNumericTraitsValuesOfObjects branch from 8deb9fc to 4d14a4b Compare February 4, 2023 17:12
Comment on lines +1277 to +1279
#define itkPrintSelfNumericTraitsMacro(name, type) \
os << indent << #name << ": " << static_cast<typename NumericTraits<type>::PrintType>(this->m_##name) << std::endl; \
ITK_MACROEND_NOOP_STATEMENT
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this idea, Jon, but I would prefer to only make limited use of macro's. In general, I think macro's make code harder to understand, and harder to debug. Specifically in this case I would at least suggest to take os and indent out of the macro definition, because it is unclear from the outside that a call to this macro "captures" these two variables.

Last December I already suggested a different approach for the implementation of those PrintSelf member functions (but I did not have time to make a PR in that direction: #3802 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Niels, it is still WIP, but here my few thoughts:

  • After STYLE: Make PrintSelf implementation style consistent #3872, I think it becomes clear that having a macro saves typing/time and allows to print all members with a consistent style: I'd dare to say that most of the times what this macro attempts to replace has not been done because it requires a non-negligible amount of typing for what can be considered to be a low reward effort at first sight, but it pays off when debugging issues, as @blowekamp made it clear in STYLE: Make PrintSelf implementation style consistent #3872 (comment).
  • The macro follows the itkPrintSelfObjectMacro philosophy, and in the same way, the os and indent are not taken as parameters as they do not change. But that is the least of the issues for now.
  • The macro is very easy to understand in this case, as it is in the itkPrintSelfObjectMacro case.

@issakomi
Copy link
Member

issakomi commented Feb 5, 2023

BTW, almost always the cast to PrintType is useless, the difference is only for char types (their print type is int) and for long double for Sun Studio 32 bit only (cast to double to avoid bug). Just FYI, there is the C++11 trick to avoid useless casts. But it really just FYI, not a request to implement.

#include <type_traits>
#include <iostream>

template<typename T, typename U, std::enable_if_t<!std::is_same<T, U>::value, int> = 0>
T conditional_static_cast(U value) noexcept
{
  std::cout << "cast" << std::endl;
  return static_cast<T>(value);
}

template<typename T, typename U, std::enable_if_t<std::is_same<T, U>::value, int> = 0>
T conditional_static_cast(U value) noexcept
{
  std::cout << "no cast" << std::endl;
  return value;
}

int main(int,char**)
{
  unsigned int x{ 1U };
  unsigned long y{ 1UL };
  unsigned long long z{ 1ULL };
  size_t a = conditional_static_cast<size_t, unsigned int>(x);
  size_t b = conditional_static_cast<size_t, unsigned long>(y);
  size_t c = conditional_static_cast<size_t, unsigned long long>(z);
  return 0;
}

@jhlegarreta
Copy link
Member Author

Thanks for the comment @issakomi. As one never knows beforehand which *Type aliases may end up adopting such types, and given our extensive use of aliases, I think what you propose will be useful to take into account for the macro in case it can be accommodated into it, and probably another reason to use it in order to save typing.

@issakomi
Copy link
Member

issakomi commented Feb 5, 2023

Some care may be required for complex types in the macro, BTW,

template <typename TComponent>
class NumericTraits<std::complex<TComponent>>
{
public:
  using Self = std::complex<TComponent>;
  // for backward compatibility
  using TheType = Self;
  using ValueType = TComponent;
  using PrintType = Self;

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:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants