STYLE: Chain consecutive insertions (`<<`) to the same output stream by N-Dekker · Pull Request #3802 · InsightSoftwareConsortium/ITK

N-Dekker · 2022-12-16T17:56:35Z

Avoiding unnecessarily mentioning one and the same output stream repetitively, when multiple consecutive statements insert into that one output stream.

Using Notepad++ v8.4.7 Regular Expressions, find ^([ ]+[^ ]+) << (.+);\r\n\1 << and replace with $1 << $2 << .

dzenanz · 2022-12-16T19:02:55Z

-  os << "} }";
-  os << ", m_BeginIndex = { ";
+  os << "} }"
+     << ", m_BeginIndex = { ";


Why didn't this fit onto the previous line?

Good question, but that's up to clang-format! Even when I remove the line-break, clang-format puts it back, in this case!

A follow-up PR could merge it together to os << "} }, m_BeginIndex = { ", of course. Clang-format would allow that on one single line.

dzenanz · 2022-12-16T19:03:10Z

-  os << indent << "Epsilon: " << m_Epsilon << std::endl;
-  os << indent << "Polarity: " << m_Polarity << std::endl;
+  os << indent << "DistanceMin: " << m_DistanceMin << std::endl
+     << indent << "DistanceMax: " << m_DistanceMax << std::endl


This change looks pointless.

It's basically what the PR is doing: avoiding to mention the output stream variable repetitively. 🤷

The idea of this PR is that rather than Pascal style WriteLine(str) statements (which print one line at a time), we could insert multiple lines into one and the same stream, by a single chain of << insertions.

But for the implementation of all those PrintSelf member functions, it might be even nicer to have some PrintData function that would write an arbitrary number of name-value pairs to the specified os, inserting the specified indent before each name-value-pair, e.g.:

PrintData(os, indent, { "Epsilon", m_Epsilon }, { "Polarity", m_Polarity }, { "DistanceMin", m_DistanceMin });

Could be implemented as a variadic template, possibly as a member function of itk::LightObject. What do you think?

This pattern appears a lot:

os << indent << "DistanceMin: " << m_DistanceMin << std::endl;

we could have a macro which makes this less prone to errors:

#define PrintMember(name) os << indent << #name ": " << m_##name << std::endl ITK_MACROEND_NOOP_STATEMENT

which would be invoked like this: PrintMember(DistanceMin);.

Update: with my latest amend, I excluded the os << insertions, which are typically from inside PrintSelf member functions. For PrintSelf a different approach may be more useful, as you suggested. Using a macro could be helpful, although I'd rather not hide away os and indent behind a macro definition.

I'm thinking of introducing a Printer class, which would support chaining multiple PrintMember calls:

Printer(os, indent).PrintMember("Epsilon", m_Epsilon).PrintMember("Polarity", m_Polarity);

And a PrintMemberMacro(name) macro, to shorten the above:

Printer{os, indent}.PrintMemberMacro(Epsilon).PrintMemberMacro(Polarity);

Note that in some cases, the macro won't work anyway, for example os << indent << "Pointer: " << static_cast<void *>(m_ImportPointer) << std::endl at

ITK/Modules/Core/Common/include/itkImportImageContainer.hxx

Line 216 in da36517

os << indent << "Pointer: " << static_cast<void *>(m_ImportPointer) << std::endl;

This might not be easy but would be amazing 🚀. Different styles might have been adopted to print e.g. matrices, etc., but we may reach a consensus. And it would definitely make the printed messages consistent: the state is now quite inconsistent, with whitespaces inserted manually (for some visual alignment of the code, and whose impact when printed is maybe unsure), = vs. :, etc. 😔.

dzenanz · 2022-12-16T19:03:20Z

-  os << indent;
-  os << "ConstNeighborhoodIterator {this= " << this;
-  os << ", m_Region = { Start = {";
+  os << indent << "ConstNeighborhoodIterator {this= " << this << ", m_Region = { Start = {";


I like this!

Your force-push removed this change :(

@N-Dekker shall I merge this PR or are there some details that still need another round of force pushes?

@jhlegarreta Thanks for asking. I think the PR is ready, although it has excluded output streams named os, as those are mostly from PrintSelf implementations. PrintSelf may be dealt with in a different way, in another PR, based on the discussion I had with Dženan (@dzenanz), starting at #3802 (comment)

Moreover, I'm not really sure if Dženan likes the idea of this PR. I mean, I'm proposing to adjust the following code:

std::cout << line1 << std::endl; std::cout << line2 << std::endl;

Replacing these two statements with a single statement:

std::cout << line1 << std::endl << line2 << std::endl;

I like it, because it does not unnecessarily repeat the name of the output stream. However, other developers may not like it, I don't know.

It's fine to me if Dženan would like to wait a little longer, to see what other people think. Anyway, thanks for your support, Jon.

OK. I'll merge it, but note that

Unless this is automated, we will not be able to enforce this automatically. A script to do this automatically would be best.

Please, go through the ITK SWG and do the corresponding changes to the coding style section. Also it is highly likely that the ITK SWG Examples were not included in this PR, so please do them in a separate PR.

Avoiding unnecessarily mentioning one and the same output stream repetitively, when multiple consecutive statements insert into that one output stream. Using Notepad++ v8.4.7 Regular Expressions, find `^([ ]+[^ ]+) << (.+);\r\n\1 << ` and replace with `$1 << $2 << `.

dzenanz · 2022-12-20T14:17:04Z

I would really like for somebody else to review this too.

jhlegarreta

Of note is that going through the modified lines there are some seemingly unnecessary blank spaces (already present prior to this commit), e.g.
67ca856#diff-40525bc7610ac6ba8f4dfedf16b2fad81460618d8f72bef57dee973cac2079fdR183
67ca856#diff-f043933bf4ca24b86407d6d96654cedd3bfc7d20a9f4ac3f6c1574d3bccc9f22R114

or inconsistent use of them to align messages in consecutive lines, or the use of colon vs. equal, but that is an issue for a separate PR. The current commit/PR does a nice job for its purpose. Thanks @N-Dekker.

dzenanz · 2022-12-23T13:52:35Z

I have minor dislike for these changes. But you liked them enough to go through the trouble of creating a PR, and another maintainer OKed it. No need to wait more - this can be merged. But I won't do it.

jhlegarreta · 2022-12-23T14:00:28Z

I have minor dislike for these changes. But you liked them enough to go through the trouble of creating a PR, and another maintainer OKed it. No need to wait more - this can be merged. But I won't do it.

👍 Cross-referencing #3802 (comment).

Merging.

jhlegarreta · 2023-01-07T01:54:10Z

A new reminder: #3802 (comment). Can this:

Be put to a script so that it can run any time/on remotes?
Be applied to all the code base, including non-default modules and examples.
Be stated in the ITK SW Guide.

Please. Thanks.

N-Dekker · 2023-01-08T11:58:55Z

A new reminder: #3802 (comment). Can this:

* Be put to a script so that it can run any time/on remotes?

* Be applied to all the code base, including non-default modules and examples.

* Be stated in the ITK SW Guide.

Thanks for the reminder, Jon. It's still on my TODO list. But if you want it to be processed more quickly, please go ahead and make a proposal! Otherwise I'll just return to this topic when I have the time.

Using regular expressions in Notepad++, finding `^([ ]+xl::xout\["\w+"\]) << (.+);\r\n\1 << ` and `^([ ]+elxout) << (.+);\r\n\1 << `, and replacing with `$1 << $2 << `. Avoids redundant internal `get_xout()` and `xoutbase::operator[]` calls. Following ITK pull request InsightSoftwareConsortium/ITK#3802 commit InsightSoftwareConsortium/ITK@67ca856 "STYLE: Chain consecutive insertions (`<<`) to the same output stream".

Avoids redundant internal `get_xout()` and `xoutbase::operator[]` calls. Using regular expressions in Notepad++, finding `^([ ]+xl::xout\["\w+"\]) << (.+);\r\n\1 << ` and `^([ ]+elxout) << (.+);\r\n\1 << `, and replacing with `$1 << $2 << `. Before doing so, removed line-breaks temporarily, by finding `^( [ ]+xl::xout\["\w+"\] << .+[^;])\r\n [ ]+<<` and `^( [ ]+elxout << .+[^;])\r\n [ ]+<<`, and replacing by `$1 <<`. Following ITK pull request InsightSoftwareConsortium/ITK#3802 commit InsightSoftwareConsortium/ITK@67ca856 "STYLE: Chain consecutive insertions (`<<`) to the same output stream".

Avoiding unnecessarily mentioning one and the same output stream repetitively, when multiple consecutive statements insert into that one output stream. Using Notepad++ v8.4.8 Regular Expressions, find `^([ ]+std::c\w\w\w) << (.+);\r\n\1 << ` and replace with `$1 << $2 << `. Following ITK pull request InsightSoftwareConsortium/ITK#3802 commit InsightSoftwareConsortium/ITK@67ca856 "STYLE: Chain consecutive insertions (`<<`) to the same output stream"

…-output-stream-insertions STYLE: Chain consecutive insertions (`<<`) to the same output stream

dzenanz reviewed Dec 16, 2022

View reviewed changes

N-Dekker force-pushed the Chain-output-stream-insertions branch from ccf89a4 to 67ca856 Compare December 20, 2022 13:38

github-actions Bot removed type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:Video Issues affecting the Video module labels Dec 20, 2022

dzenanz requested review from blowekamp and jhlegarreta December 20, 2022 14:16

jhlegarreta approved these changes Dec 21, 2022

View reviewed changes

N-Dekker marked this pull request as ready for review December 21, 2022 14:47

jhlegarreta merged commit ac8fde0 into InsightSoftwareConsortium:master Dec 23, 2022

N-Dekker mentioned this pull request Feb 4, 2023

ENH: Use a macro to print numeric traits values of objects #3909

Draft

2 tasks

hjmjohnson pushed a commit to hjmjohnson/ITK that referenced this pull request May 6, 2026

Merge pull request InsightSoftwareConsortium#3802 from N-Dekker/Chain…

39d967b

…-output-stream-insertions STYLE: Chain consecutive insertions (`<<`) to the same output stream

Uh oh!

Conversation

N-Dekker commented Dec 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

N-Dekker Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Dec 20, 2022

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

dzenanz commented Dec 23, 2022

Uh oh!

jhlegarreta commented Dec 23, 2022

Uh oh!

jhlegarreta commented Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

N-Dekker commented Jan 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

N-Dekker Dec 16, 2022 •

edited

Loading

jhlegarreta commented Jan 7, 2023 •

edited

Loading