Skip to content

BUG: Fix printing of values in MetaDataObject #4814

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 23, 2024
Merged

BUG: Fix printing of values in MetaDataObject #4814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 2 additions & 55 deletions Modules/Core/Common/include/itkMetaDataObject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,8 @@ namespace itk
* and redefining the copy constructor and initializing constructor and the Get/Set functions
* to work around those deficiencies.
*
* The behavior of the MetaDataObject<Type>::Print() function has many plausible
* application dependent implementations. The default implementation prints the
* string "[UNKNOWN PRINT CHARACTERISTICS]" that works for all possible
* MetaDataObject types.
*
* The application developer may overload the default implementation to provide
* a specialized Print() characteristics to produce results desirable for their application.
* A set of very crude Macros {NATIVE_TYPE_METADATAPRINT, ITK_OBJECT_TYPE_METADATAPRINT_1COMMA,
* ITK_IMAGE_TYPE_METADATAPRINT } are provided to facilitate a very simple implementation, and as an example.
* The default implementation prints uses the MetaDataObjectType's Print or operator<< if available. Otherwise, it
* prints string "[UNKNOWN PRINT CHARACTERISTICS]".
*
* \ingroup ITKCommon
*
Expand Down Expand Up @@ -264,52 +257,6 @@ ExposeMetaData(const MetaDataDictionary & Dictionary, const std::string key, T &

} // end namespace itk

/**
* \def ITK_NATIVE_TYPE_METADATAPRINT( TYPE_NAME )
* \brief An ugly macro to facilitate creating a simple implementation of
* the MetaDataObject<Type>::Print() function for types that
* have operator<< defined.
* \param TYPE_NAME the native type parameter type
*/
#define ITK_NATIVE_TYPE_METADATAPRINT(TYPE_NAME) \
template <> \
void itk::MetaDataObject<TYPE_NAME>::Print(std::ostream & os) const \
{ \
os << this->m_MetaDataObjectValue << std::endl; \
}

/**
* \def ITK_OBJECT_TYPE_METADATAPRINT_1COMMA( TYPE_NAME_PART1, TYPE_NAME_PART2 )
* \brief An ugly macro to facilitate creating a simple implementation of
* the MetaDataObject< Type >::Print() function for
* itk::Objects that have 1 comma in their type definition
* \param TYPE_NAME_PART1
* \param TYPE_NAME_PART2
*/
#define ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(TYPE_NAME_PART1, TYPE_NAME_PART2) \
template <> \
void itk::MetaDataObject<TYPE_NAME_PART1, TYPE_NAME_PART2>::Print(std::ostream & os) const \
{ \
this->m_MetaDataObjectValue->Print(os); \
}

/**
* \def ITK_IMAGE_TYPE_METADATAPRINT( STORAGE_TYPE )
* An ugly macro to facilitate creating a simple implementation of
* the MetaDataObject<Type>::Print() function for
* itk::Image\<STORAGE_TYPE,[1-8]\>\::Pointer
* \param STORAGE_TYPE The storage type of the image type to print.
*/
#define ITK_IMAGE_TYPE_METADATAPRINT(STORAGE_TYPE) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 1>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 2>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 3>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 4>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 5>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 6>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 7>::Pointer) \
ITK_OBJECT_TYPE_METADATAPRINT_1COMMA(itk::Image<STORAGE_TYPE, 8>::Pointer)

#ifndef ITK_MANUAL_INSTANTIATION
# include "itkMetaDataObject.hxx"
#endif
Expand Down
36 changes: 35 additions & 1 deletion Modules/Core/Common/include/itkMetaDataObject.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,45 @@ MetaDataObject<MetaDataObjectType>::SetMetaDataObjectValue(const MetaDataObjectT
Self::Assign(m_MetaDataObjectValue, newValue);
}

namespace
{
Comment on lines +62 to +63
Copy link
Contributor

@N-Dekker N-Dekker Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Please don't open an unnamed namespace in a header file (*.h, *.hxx). The compiler should have produced a warning! C++ Core Guidelines: Don’t use an unnamed (anonymous) namespace in a header

Copy link
Member

Choose a reason for hiding this comment

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

Looks like a good excuse for an immediate follow-up? 😄 Is there also some chance for you tackle std::vector printing?

Copy link
Member Author

Choose a reason for hiding this comment

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

@N-Dekker What is the alternative suggestion? Should move the classes to ITK's meta programming sub namespace?

Copy link
Contributor

@N-Dekker N-Dekker Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should move the classes to ITK's meta programming sub namespace?

Good question... is the intention to have has_Print and has_output_operator as part of the public interface, for end-users? Then they could go into the meta programming sub-namespace. Or they could just remain in the root of the itk namespace. But then they should still be moved from the ".hxx" file into a ".h" file

Otherwise, is the intention to make clear to users that has_Print and has_output_operator are just implementation details? Then we could place them into some implementation-detail-namespace. We could for example name the namespace "itkMetaDataObjectImplementationDetail".

A third option: If you want to entirely hide them away from the end-users, you could probably make them privately nested types of MetaDataObject.

template <class T, class = void>
struct has_Print : std::false_type
{};

template <class T>
struct has_Print<T, std::void_t<decltype(std::declval<T>().Print(std::declval<std::ostream &>()))>> : std::true_type
{};

template <class T, class = void>
struct has_output_operator : std::false_type
{};

template <class T>
struct has_output_operator<T, std::void_t<decltype(std::declval<std::ostream &>() << std::declval<T>())>>
: std::true_type
{};
} // namespace

template <typename MetaDataObjectType>
void
MetaDataObject<MetaDataObjectType>::Print(std::ostream & os) const
{
Superclass::Print(os);
// future c++20 feature
// constexpr bool hasPrint = false; requires( const &MetaDataObjectType obj ) { obj.Print(os); };

if constexpr (has_Print<MetaDataObjectType>::value)
{
m_MetaDataObjectValue.Print(os);
}
else if constexpr (has_output_operator<MetaDataObjectType>::value)
{
os << m_MetaDataObjectValue;
}
else
{
Superclass::Print(os);
}
}

} // end namespace itk
Expand Down
7 changes: 6 additions & 1 deletion Modules/Core/Common/test/itkMetaDataObjectTest.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ itkMetaDataObjectTest(int, char *[])
result += testMetaData<float>(-24);
result += testMetaData<double>(-24);
result += testMetaData<std::string>("I T K");

result += testMetaData<std::vector<double>>({ 1.0, 2.0, 3.0 });
result += testMetaData<std::vector<std::vector<double>>>({ { 1.0, 2.0, 3.0 }, { 4.0, 5.0, 6.0 } });
result += testMetaData(itk::Array<char>(3, 'I'));
result += testMetaData<itk::Array<double>>(itk::Array<double>(3, 3.0));
result += testMetaData(itk::Matrix<float, 4, 4>());
result += testMetaData<itk::Matrix<double, 3, 3>>(itk::Matrix<double, 3, 3>::GetIdentity());
using ImageType = itk::Image<unsigned short, 3>;
ImageType::Pointer image = nullptr;
result += testMetaData<ImageType::Pointer>(image);
Expand Down