-
-
Notifications
You must be signed in to change notification settings - Fork 275
Stop trying to use std::filesystem.
#1109
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
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 removes all std::filesystem usage from libpqxx due to portability concerns and implementation complexities. The library no longer provides support for std::filesystem::path in its APIs.
- Removed
std::filesystem::pathconstructor fromzviewclass - Removed test coverage for
std::filesystem::pathsupport - Updated documentation to clarify that libpqxx no longer uses
std::filesystem
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/test_blob.cxx | Removed #include <filesystem> and the test function test_blob_accepts_std_filesystem_path() along with its registration |
| include/pqxx/zview.hxx | Removed #include <filesystem> and the constructor accepting std::filesystem::path |
| UPGRADING.md | Updated build changes section to clarify that libpqxx itself no longer uses std::filesystem |
The changes are clean, consistent, and complete. All references to std::filesystem have been properly removed from the API surface, tests, and documentation. The removal is straightforward - the zview class previously offered a convenience constructor for std::filesystem::path (only on non-Windows platforms), and this has been cleanly removed along with its test coverage. Users can still pass file paths as C-style strings or std::string objects to functions that accept zview.
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
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
There were problems with this that I couldn't figure out, and from more reading, I get the impression that there's nobody designing it who does know exactly how it's supposed to work. There's no magic solution to portability problems there, just new and unknown intricacies to replace the old, somewhat-known ones.
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
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
There were problems with this that I couldn't figure out, and from more reading, I get the impression that there's nobody designing it who does know exactly how it's supposed to work. There's no magic solution to portability problems there, just new and unknown intricacies to replace the old, somewhat-known ones.