Skip to content

Conversation

blowekamp
Copy link
Member

@blowekamp blowekamp commented Aug 21, 2024

Also removes dead code related to METADATAPRINT

Does not currently add printing for std::vector.

Fix #1454
Near duplicate of: #4368
Co-Author: Pablo Hernandez-Cerdan [email protected]

PR Checklist

  • No API changes were made (or the changes have been approved)
  • No major design changes were made (or the changes have been approved)
  • Added test (or behavior not changed)
  • Updated API documentation (or API not changed)
  • Added license to new files (if any)
  • Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
  • Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

Also removes dead code related to METADATAPRINT

Fix InsightSoftwareConsortium#1454

Co-Author: Pablo Hernandez-Cerdan <[email protected]>
@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Core Issues affecting the Core module labels Aug 21, 2024
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

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

Code looks elegant. Hopefully it works!

@blowekamp
Copy link
Member Author

This also does not make use of itk::Matrix::PrintSelf. The LightObject class provides a standard Print, and PrintSelf method ( with indent). The MetaDataObject class is derived from the LightObject, while the MetaDataDictionary is not.

@dzenanz
Copy link
Member

dzenanz commented Aug 21, 2024

Do the image directions and other matrices get printed correctly or as [UNKNOWN PRINT CHARACTERISTICS]?

@blowekamp
Copy link
Member Author

The itk::Matrix and itk::Array classes get printed with their "operator<<". The new Matrix::PrintSelf (#4772) is not used.

std::vector still get printed with [UNKNOWN PRINT CHARACTERISTICS]

@blowekamp blowekamp marked this pull request as ready for review August 21, 2024 20:54
@phcerdan
Copy link
Contributor

phcerdan commented Aug 22, 2024

Looks good and thanks for the follow up.
Why aren't we adding support for std::vector or other iterables?
Seems like a good opportunity.

PS: In any case, this looks good as it is. It fixes the bug. No need to go for the generalization to print other iterables. But maybe good to have it in mind for the future.

@phcerdan phcerdan self-requested a review August 22, 2024 12:23
Copy link
Contributor

@phcerdan phcerdan left a comment

Choose a reason for hiding this comment

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

Looks good!
In the future, we could look up on how to print iterables in a general way.

@dzenanz
Copy link
Member

dzenanz commented Aug 22, 2024

Could this operator be used?

@dzenanz dzenanz requested a review from hjmjohnson August 22, 2024 12:41
@blowekamp
Copy link
Member Author

Could this operator be used?

I ran into issue with the two approaches I tried to use that overloaded operator. Perhaps this can be a second PR?

@blowekamp blowekamp requested a review from N-Dekker August 22, 2024 22:59
@dzenanz dzenanz merged commit 37fca10 into InsightSoftwareConsortium:master Aug 23, 2024
Comment on lines +62 to +63
namespace
{
Copy link
Contributor

@N-Dekker N-Dekker Aug 23, 2024

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

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.

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:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MetaDataDictionary::Print only prints [UNKNOWN_PRINT_CHARACTERISTICS]

4 participants