Skip to content

Add cross-platform file I/O utilities - #660

Draft
Conrad Johnston (ConradJohnston) wants to merge 26 commits into
mainfrom
feature/cross-platform-file-io
Draft

Add cross-platform file I/O utilities#660
Conrad Johnston (ConradJohnston) wants to merge 26 commits into
mainfrom
feature/cross-platform-file-io

Conversation

@ConradJohnston

Copy link
Copy Markdown
Contributor

Summary

  • add shared C++ and Python helpers for reliable text reads and atomic file replacement
  • handle paths, permissions, cleanup, and Windows read-only files consistently
  • add focused cross-platform regression tests

First step toward #655.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

# Conflicts:
#	cpp/src/qdk/chemistry/utils/CMakeLists.txt
#	python/src/qdk_chemistry/utils/__init__.py
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds shared C++ and Python file I/O utilities as the first step toward #655.

Changes:

  • Adds atomic file replacement, text reading, and parent-directory helpers.
  • Handles permissions, Unicode paths, Windows read-only files, and cleanup.
  • Adds extensive cross-platform regression tests.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
cpp/include/qdk/chemistry/utils/file_io.hpp Declares the C++ API.
cpp/src/qdk/chemistry/utils/file_io.cpp Implements C++ file operations.
cpp/src/qdk/chemistry/utils/CMakeLists.txt Adds the implementation to the library.
cpp/tests/test_file_io.cpp Adds C++ regression tests.
python/src/qdk_chemistry/utils/file_io.py Implements Python file operations.
python/src/qdk_chemistry/utils/__init__.py Exports the Python API.
python/tests/test_utils_file_io.py Adds Python regression tests.
Suppressed comments (2)

cpp/tests/test_file_io.cpp:44

  • Several Windows tests intentionally leave files with FILE_ATTRIBUTE_READONLY, which causes std::filesystem::remove_all to fail. Ignoring that error leaks the per-test directory and masks cleanup regressions. Clear read-only attributes during teardown and report a failure if the final removal fails.
  void TearDown() override {
    std::error_code ignored;
    std::filesystem::remove_all(root_, ignored);

cpp/tests/test_file_io.cpp:612

  • This expectation is not valid on Windows. The reserved handle remains open after remove, so the file is delete-pending and the ofstream cannot recreate the same pathname; after the reservation closes, no replacement remains. The Python counterpart explicitly skips this assertion on Windows. Guard this assertion likewise, or assert the platform-specific outcome.
  EXPECT_TRUE(std::filesystem::exists(replacement_path));

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/src/qdk_chemistry/utils/file_io.py Outdated
Comment thread cpp/tests/test_file_io.cpp Outdated
Comment on lines +35 to +39
root_ = std::filesystem::temp_directory_path() /
("qdk_file_io_test_" +
std::to_string(
std::chrono::steady_clock::now().time_since_epoch().count()));
std::filesystem::create_directories(root_);
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

2 participants