WIP: Add a PrintHelper file to overload boolean and vector printing.

[hjmjohnson]

No description provided.

[hjmjohnson]

http://review.source.kitware.com/#/c/22069/5

Jon Hait... Gorroño
Uploaded patch set 1.Feb 12, 2017

Jon Hait... Gorroño
Patch Set 1: Very first version. The approach is very likely not to be the best one. I used macros instead of classes, and I'm seeing its limitations. If you definitely think we should use classes, feel free to continue the discussion. Thanks.Feb 12, 2017

Kitware Build Robot
Patch Set 1: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22069-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Feb 12, 2017

Jon Hait... Gorroño
Uploaded patch set 2.Sep 23 1:10 PM

Jon Hait... Gorroño
Patch Set 2: Actually, cross-referencing the comment here: http://review.source.kitware.com/#/c/23725/1 I had started this a long, long time ago, but did not invest more time on it :-S Some comments: Brad's addition was actually being exercised according to the coverage. However, a quick search for itk::ImageIOBase::Pointer gave: ITK\Modules\Filtering\FFT\test\itkComplexToComplexFFTImageFilterTest.cxx(103) ITK\Modules\Filtering\FFT\test\itkVnlComplexToComplexFFTImageFilterTest.cxx(85) ITK\Modules\IO\GE\test\itkGEImageIOTest.cxx(91) ITK\Modules\IO\ImageBase\test\itkImageIOBaseTest.cxx(200) ITK\Modules\IO\ImageBase\test\itkVectorImageReadWriteTest.cxx(115) ITK\Modules\Filtering\ImageGradient\test\itkGradientImageFilterTest2.cxx(112) ITK\Modules\Segmentation\LabelVoting\test\itkVotingBinaryImageFilterTest.cxx(100) The one in itkImageIOBaseTest cannot be used because the instance is null. For the rest, the problem is that the base class is only known at runtime, so we cannot do: EXERCISE_BASIC_OBJECT_METHODS( ioBase, ioBase->GetNameOfClass(), LightProcessObject ); So I could not really check that the solution was working. If anyone has suggestions, I'd be happy to try. As for the booleans, I simply did not find the way to overload the << operator without incurring in a "more than one operator matches these operands" error. Thanks.Sep 23 1:21 PM

Kitware Build Robot
Patch Set 2: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22069-2&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Sep 23 1:22 PM

Jon Hait... Gorroño
Uploaded patch set 3.Sep 23 1:40 PM

Kitware Build Robot
Patch Set 3: Verified-1 Build Failed: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22069-3&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Sep 23 1:53 PM

Jon Hait... Gorroño
Uploaded patch set 4.Sep 23 2:04 PM

Kitware Build Robot
Patch Set 4: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=22069-4&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Sep 23 2:16 PM

Dzenan Zukic
Patch Set 4: Code-Review+2 Excellent! The next step would be to find instanced of "manually" printing vectors, and simplify them by using this mechanism. But that should be a new patch!Sep 24 9:02 AM

Hans J. Johnson
Patch Set 4: Code-Review+1 Let's get this out of WIP mode.Sep 24 9:20 AM

Jon Hait... Gorroño
Uploaded patch set 5: Commit message was updated.Sep 25 7:43 PM

Jon Hait... Gorroño
Patch Set 5: I updated the commit message. Dzenan, definitely, yes, I'll need to look for instances of PrintSelf where this can save code. Also, I'll take the work of adding more features (lists, maps, booleans) to another topic.Sep 25 7:44 PM

Dzenan Zukic
Patch Set 5: Code-Review+2Sep 26 7:50 AM

Bradley Lowekamp
Patch Set 5: With this approach all overloaded operator<< done in the itk name space scope will get this overload. Perhaps we should consider requiring to opt in to the the "print helpers". If we place them is a sub-namespace say "itk::print_helper". The we could just add "using print_helper" to the PrintSelf method to opt into using these methods. Similarly, outside programs could do a "using itk::print_helper" and get these overloaded operators too.Sep 26 8:05 AM

Jon Hait... Gorroño

[hjmjohnson]

@jhlegarreta
@blowekamp stated "With this approach all overloaded operator<< done in the itk name space scope will get this overload. Perhaps we should consider requiring to opt in to the the "print helpers". If we place them is a sub-namespace say "itk::print_helper". The we could just add "using print_helper" to the PrintSelf method to opt into using these methods. Similarly, outside programs could do a "using itk::print_helper" and get these overloaded operators too."

Can this be completed?

[jhlegarreta]

Thanks for the comment; still in my ToDo list.

[hjmjohnson]

Accidently deleted/closed branch when the script for moving from Gerrit went haywire.

[kwrobot-v1]

Errors:

• Failed to reserve ref 271673c for the merge request: invalid git ref: 'no such commit'.
• Failed to run the checks: git error: failed to list commits on the merge request https://github.com/InsightSoftwareConsortium/ITK/pull/103: fatal: bad object 271673cdcd960b337b5d31507858b28f1e9417f9 .

[hjmjohnson]

@jhlegarreta This seems like some low-hanging fruit. Any change it can be taken to completion and off the to todo list?

[jhlegarreta]

@hjmjohnson thanks for the interest.

I'd love to get this done, see it merged, and reduce my ToDo list by one item. For this to happen, I need the help from a few though, since the best design choice to get this done is still unclear to me.

[phcerdan]

I don't think overload of operator << of classes that ITK does not own is a good practice

What about anstd::string itk::to_string(const std::vector &) function in this header? There is a std::to_string already, but only for scalars.

Usage:

std::cout << "My vector: " << itk::to_string(myVector) << std::endl;

[blowekamp]

@phcerdan I think we are in some agreement that by default overloading operator<< and making it default accessible is not the best thing to do. I'm not sure how much weight to put into that one SO response, here is a blog article on doing it.

My suggestions is to:

namespace itk {
namespace to_string {
	template< typename T >
	std::ostream & operator<<( std::ostream & os, const std::vector< T >& v );
}
}

Then usage would be:

namespace itk
{
 void something(void)
{
using to_string;
std::cout << "My vector: " << myVector << std::endl;
}
}

For both these approaches argument dependent lookup does not work, since we can't (must not) put the function/operator into the std namespace.

[jhlegarreta]

Thanks @phcerdan @blowekamp !

Both approaches seem fair to me, and I have no preference, beyond may be keeping the code style consistent.

IMHO, Brad's solution is cleaner when calling the stream insertion operator.

However, I ignore whether placing the using to_string in the corresponding method (usually, this will be intended to be used in the PrintSelf members) is a price to pay (in terms of code readability or style) that we all agree on.

I ignore whether Pablo's solution has any downside or caveat.

[phcerdan]

Yeah, Brad's solution sounds good to me, thanks for that Brad.
After reading more about it, using a function instead of a iostream has the caveat of not being able to control the number of digits used in the conversion in an easy way.

+1 for putting the << overload in itk::print_helpers namespace or similar.

[jhlegarreta]

Thanks for the comments and suggestions !

I'll then come up with a new patch set hopefully by the end of the week.

[jhlegarreta]

Superseded by #512. Closing.