Skip to content

Refactor various filesystem related operations to use std::filesystem#309

Open
nightlark wants to merge 8 commits intofelt:mainfrom
nightlark:use-std-filesystem
Open

Refactor various filesystem related operations to use std::filesystem#309
nightlark wants to merge 8 commits intofelt:mainfrom
nightlark:use-std-filesystem

Conversation

@nightlark
Copy link
Contributor

This change makes use of the std::filesystem API introduced in C++17, and replaces a few instances where variable length arrays and snprintf were being used to construct temporary file paths with std::string (several other places were already using std::string). Using std::filesystem should be a bit more portable than the POSIX function calls that were previously used, which could help get a working native/MinGW build on Windows.

  • Remove #include <sys/stat.h> from files where it is no longer needed
  • Use std::filesystem functions to check free disk space available, create directories, if a path exists, if a path is a directory, and file sizes
  • Use std::filesystem::remove instead of unlink (behavior on POSIX should be identical)
  • Use std::string data() method to get a mutable char *, instead of casting away the const-ness of c_str() (since C++11, the array returned by data() is null-terminated)
  • Replace use of VLAs for temporary file path construction with std::string
  • Replace hard-coded use of /tmp for temporary directory with call to std::filesystem -- on Linux/macOS this returns /tmp by default unless the user sets an environment variable to override it (TMPDIR, TMP, TEMP, TEMPDIR), and on Windows this returns the same thing as GetTempPath

@nightlark
Copy link
Contributor Author

nightlark commented Jan 29, 2025

Any reviewers have questions, feedback or requested changes (or just haven't had time to review yet)?

Also happy to set up a zoom/webex/teams call to go through the various changes if that would help.

@nightlark
Copy link
Contributor Author

@e-n-f any feedback on these changes?

@nightlark
Copy link
Contributor Author

Hi! It's been a year, any thoughts on this @e-n-f, @dnomadb, @ChrisLoer, @arredond, or @migurski?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant