Skip to content

style(InputTextStream): Use std::unique_ptr<std::ifstream> #1425

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

Conversation

N-Dekker
Copy link
Contributor

Avoided manual memory management. Followed the C++ Rule of Zero, by removing the user declared default-constructor and destructor of InputTextStream.

Avoided manual memory management. Followed the C++ Rule of Zero, by removing the
user declared default-constructor and destructor of `InputTextStream`.
@N-Dekker N-Dekker force-pushed the InputTextStream-unique_ptr-ifstream branch from 700d33e to 6f9573b Compare July 18, 2025 16:21
@thewtex thewtex requested a review from Copilot July 21, 2025 21:16
Copy link
Contributor

@Copilot Copilot AI left a 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 InputTextStream class by replacing manual memory management with std::unique_ptr<std::ifstream>. The changes eliminate the need for explicit destructor and constructor declarations, following the C++ Rule of Zero principle.

  • Replaced raw pointer and boolean flag with std::unique_ptr<std::ifstream>
  • Removed manual new/delete operations and associated tracking logic
  • Simplified the class interface by removing user-declared constructor and destructor

delete m_IStream;
}
m_WasmStringStream = nullptr;
m_InputFileStream = std::make_unique<std::ifstream>(fileName);
Copy link
Preview

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

The file stream is created without checking if the file was successfully opened. Consider adding error handling to verify the stream state after creation, or document that callers should check the stream state via GetPointer().

Suggested change
m_InputFileStream = std::make_unique<std::ifstream>(fileName);
m_InputFileStream = std::make_unique<std::ifstream>(fileName);
if (!m_InputFileStream->is_open())
{
throw std::ios_base::failure("Failed to open file: " + fileName);
}

Copilot uses AI. Check for mistakes.

delete m_IStream;
}
m_WasmStringStream = nullptr;
m_InputFileStream = std::make_unique<std::ifstream>(fileName);
Copy link
Preview

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

[nitpick] Consider adding the std::ifstream::in flag for consistency with the previous implementation, even though it's the default for ifstream: std::make_unique<std::ifstream>(fileName, std::ifstream::in)

Suggested change
m_InputFileStream = std::make_unique<std::ifstream>(fileName);
m_InputFileStream = std::make_unique<std::ifstream>(fileName, std::ifstream::in);

Copilot uses AI. Check for mistakes.

@N-Dekker
Copy link
Contributor Author

This pull request (#1425) is superseded by the following pull request:

@N-Dekker N-Dekker closed this Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant