-
-
Notifications
You must be signed in to change notification settings - Fork 661
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
BUG: Fix printing of values in MetaDataObject #4368
base: master
Are you sure you want to change the base?
BUG: Fix printing of values in MetaDataObject #4368
Conversation
e7bce87
to
c1b352c
Compare
Also removes dead code related to METADATAPRINT Fix InsightSoftwareConsortium#1454
c1b352c
to
1afdbce
Compare
|
||
namespace itk | ||
{ | ||
template <typename TIterable> | ||
void | ||
printIterable(std::ostream & os, const TIterable & iterable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting pull request, thanks @phcerdan
Can you possibly hide printIterable
(or whatever its name will be) from the public API? Because it isn't intended to be exposed directly to the end user, right? I guess it should be possible to make it a private (preferably static
) member function of MetaDataObject
. Otherwise, it might be possible to define it as a lambda, hidden locally inside MetaDataObject::PrintValue
, but that seems more challenging to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you can make it private, you might as well just name the function something like PrintHelper
, rather than printIterable
. My 2 cent!
template <typename T, typename = void> | ||
inline constexpr bool is_iterable_print_v = false; | ||
// Specialize for std::vector<T> and std::vector<std::vector<T>> | ||
template <typename T> | ||
inline constexpr bool is_iterable_print_v<std::vector<T>, std::void_t<>> = true; | ||
template <typename T> | ||
inline constexpr bool is_iterable_print_v<std::vector<std::vector<T>>, std::void_t<>> = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to see a use case for variable templates, thanks @phcerdan. Unfortunately "itkMetaDataObject.hxx" is usually #include
'd (indirectly) by end users, and I guess it's not the intention to add is_iterable_print_v
to the global namespace of the user, right? If you agree, you may possibly make is_iterable_print_v
a private static member of MetaDataObject
. Or otherwise, maybe it's simpler to just add overloaded private static MetaDataObject::IsIterablePrint(const T&)
member functions. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points @N-Dekker, I will make static private members
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCC doesn't like it... (moving is_iterable_print
inside the class).
https://stackoverflow.com/questions/72190700/explicit-template-argument-list-not-allowed-with-g-but-compiles-with-clang
I will explore further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fixed in gcc14, but we cannot afford that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option I guess would be to have constexpr static member functions IsIterablePrint(const T&)
overloaded (rather than "specialized") for std::vector<T>
and std::vector<std::vector<T>>
, right?
You could then use it like:
if constexpr (IsIterablePrint(iterable))
I guess 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the name of the boolean, maybe something like ShouldBePrintedElementWise
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am very supportive of these efforts! Modern C++ can accomplish the desired task much more cleanly and robustly.
I appreciate @N-Dekker comments to fortify the API (avoid leaky API) and would like to see those comments addressed before finalizing this PR.
Also removes dead code related to METADATAPRINT
Fix #1454
PR Checklist