-
-
Notifications
You must be signed in to change notification settings - Fork 713
ENH: Add stream insertion operators for std::list and C-style arrays #5616
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.
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
base: main
Are you sure you want to change the base?
Conversation
66eeadd to
78b3556
Compare
78b3556 to
557f7b7
Compare
std::list and C-style arrays to print_helper
557f7b7 to
ce3babd
Compare
|
Interesting! Is the commit message also generated by AI? The body says:
That's not a complete English sentence 🤷 |
| { | ||
| if (l.empty()) | ||
| { | ||
| return os << "()"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally we printed square brackets "[]", whereas Copilot proposes to print parentheses, "()". Is that an intended behavior change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to distinguish arbitrarily-sized lists from fixed-length arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to distinguish arbitrarily-sized lists from fixed-length arrays?
Like resizable Python lists [ 1, 2, 3 ] versus fixed-length Python tuples ( 1, 2, 3 )? Doesn't sound unreasonable to me. 👍
But if we would follow Python, std::list should still have square brackets anyway 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use square brackets [] for resizable containers (std::vector and std::list) and parentheses () for fixed-size C-style arrays. This matches the original ITK behavior and provides a visual distinction between resizable and fixed-size containers. (commit 557d2e4)
| auto it = l.begin(); | ||
| auto last = std::prev(l.end()); | ||
| for (; it != last; ++it) | ||
| { | ||
| os << *it << ", "; | ||
| } | ||
| return os << *it << ')'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preferably do the declaration of the iterator it within the for:
for (auto it = l.begin(); it != last; ++it)
{
os << *it << ", ";
}
return os << *last << ')';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also consider the implementation of the similar operator<<(std::ostream & os, const std::vector<T> & v) in the very same source file:
ITK/Modules/Core/Common/include/itkPrintHelper.h
Lines 48 to 50 in 1cb6591
| os << '('; | |
| std::copy(v.begin(), v.end() - 1, std::ostream_iterator<T>(os, ", ")); | |
| return os << v.back() << ')'; |
So I would suggest using std::copy and std::ostream_iterator<T>(os, ", ") when printing std::list as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented both suggestions: now using std::copy with std::ostream_iterator<T>(os, ", ") and simplified the implementation to match the vector version. (commit 557d2e4)
|
I manually removed the false association to 1177. |
Thanks, @dzenanz Do you have a clue why Copilot did mention 1177? |
|
I don't know. |
I see this as an interesting experiment, not a regular pull request. It's a way to get to know more about the strengths and the limitations of Copilot. Copilot produces quite some relevant code, but it also appears to produce non-sense, now and then. Like the reference to an unrelated issue, as I mentioned at #5616 (comment) The code quality from Copilot isn't always good enough. It's certainly not perfect. But I think it can be used as a starting point. Currently I think Copilot generated code should be reviewed with extra care. I think we should have more reviewers on Copilot PRs than on human PRs. We cannot just trust Copilot, the way we might trust a human programmer. So in practice I would still prefer a human PR 🤷 |
|
@N-Dekker I agree 100%. My experience is that Co-Pilot never has the correct solution, but it often has a useful starting point. |
|
AI certainly makes mistakes (as do humans!) Copilot can be instructed to follow-up using copilot. More guidance here @copilot address the failing tests |
dzenanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashing commits and rewriting the commit message is all it takes for this to be merged. Do you agree?
dzenanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Squashing commits and rewriting the commit message is all it takes for this to be merged. Do you agree?
|
I now see the test failure: |
ITK's
PrintSelfmethods contain manual loops to print containers likestd::listand C-style arrays because these types lack stream insertion operators in theprint_helpernamespace.Changes
Enhanced
itkPrintHelper.hoperator<<forstd::list<T>usingstd::copyandstd::ostream_iterator(consistent with vector implementation)operator<<for C-style arraysT[N]with SFINAE to excludechar[](prevents ambiguity with string literals)std::vector,std::list) use square brackets[], fixed-size arrays use parentheses()Updated PrintSelf methods
Replaced manual iteration loops with direct stream insertion using
using namespace itk::print_helper:m_StrideTable,m_OffsetTablem_CirclesListm_LinesListm_Scale,m_MatrixScaleTests
itkPrintHelperGTest.cxxcovering vectors, lists, arrays, empty containers, and nested typesExample
Before:
After:
Design Decisions
Bracket Style:
Following Python-like conventions and maintaining backward compatibility:
[]: Resizable containers (std::vector,std::list) - matches original ITK behavior(): Fixed-size arrays - distinguishes from resizable containersImplementation:
std::listusesstd::copyandstd::ostream_iteratorlikestd::vectorfor consistencyFixes #513
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.