-
Notifications
You must be signed in to change notification settings - Fork 56
style: Use std::unique_ptr<std::istream> in InputStreamBase
#1432
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
style: Use std::unique_ptr<std::istream> in InputStreamBase
#1432
Conversation
14ff273 to
f3ed77f
Compare
Simplified the class by removing its member variables m_DeleteIStream and m_WasmStringStream. Avoided manual memory management.
Moved the implementation of non-trivial member functions of InputStreamBase (including its defaulted destructor, which deallocates dynamic memory) from its h file to its cxx file. Might improve the compilation speed.
f3ed77f to
965f201
Compare
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.
Pull Request Overview
This PR modernizes the InputStreamBase class by replacing manual memory management with std::unique_ptr<std::istream>, eliminating the need for manual cleanup and simplifying the class design.
Key changes:
- Replaced raw pointer
m_IStreamwithstd::unique_ptr<std::istream> - Removed manual memory management variables
m_DeleteIStreamandm_WasmStringStream - Moved method implementations from header to source file
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| include/itkInputStreamBase.h | Replaced raw pointer with unique_ptr, removed manual memory management members, moved method declarations |
| src/itkInputStreamBase.cxx | Added method implementations using unique_ptr and modern C++ practices |
| { | ||
| const auto wasmStringStream = WasmStringStream::New(); | ||
| wasmStringStream->SetJSON(json.c_str()); | ||
| m_IStream = std::make_unique<std::stringstream>(std::move(wasmStringStream->GetStringStream())); |
Copilot
AI
Jul 25, 2025
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.
This line attempts to move-construct a stringstream from another stringstream, but the GetStringStream() method likely returns a reference, not a movable object. This could result in compilation errors or unintended copying. Consider using a different approach to transfer the stream data.
| m_IStream = std::make_unique<std::stringstream>(std::move(wasmStringStream->GetStringStream())); | |
| std::stringstream tempStream = wasmStringStream->GetStringStream(); | |
| m_IStream = std::make_unique<std::stringstream>(tempStream.str()); |
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.
The change suggested by Copilot does not compile. Specifically:
std::stringstream tempStream = wasmStringStream->GetStringStream();This suggestion would try to copy-construct a std::stringstream, but std::stringstream is not copy-constructible!
std::stringstream is move-construcible, so the stringstream of a WasmStringStream object can be properly moved into another stringstream, as in the proposed PR. Especially because the WasmStringStream object itself is no longer used afterwards.
thewtex
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.
@N-Dekker thanks!
Simplified the class by removing its member variables m_DeleteIStream and m_WasmStringStream. Avoided manual memory management.