BUG: Fix printing of values in MetaDataObject#4368
BUG: Fix printing of values in MetaDataObject#4368phcerdan wants to merge 1 commit intoInsightSoftwareConsortium:masterfrom
Conversation
github-actions
bot
added
type:Bug
phcerdan
force-pushed
the
fix_metadataobject_print
branch
from
e7bce87 to
c1b352c
Compare
phcerdan
added
the
action:ApplyClangFormat
Also removes dead code related to METADATAPRINT Fix InsightSoftwareConsortium#1454
phcerdan
force-pushed
the
fix_metadataobject_print
branch
from
c1b352c to
1afdbce
Compare
| { | ||
| template <typename TIterable> | ||
| void | ||
| printIterable(std::ostream & os, const TIterable & iterable) |
There was a problem hiding this comment.
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.
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.
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.
Good points @N-Dekker, I will make static private members
There was a problem hiding this comment.
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.
It seems fixed in gcc14, but we cannot afford that.
There was a problem hiding this comment.
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.
Regarding the name of the boolean, maybe something like ShouldBePrintedElementWise ?
hjmjohnson
left a comment
hjmjohnson
left a comment
There was a problem hiding this comment.
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.
|
Should this be closed, given that #4814 was merged? |
5 participants
Also removes dead code related to METADATAPRINT
Fix #1454
PR Checklist