Skip to content
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

Slightly more std::filesystem for nix eval #11650

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

Context

Progress on #9205

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Oct 7, 2024
Comment on lines +14 to +15
namespace nix::fs { using namespace std::filesystem; }

Copy link
Member

Choose a reason for hiding this comment

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

nitpick(non-blocking): Could maybe even move this inside of the class declaration.

Suggested change
namespace nix::fs { using namespace std::filesystem; }
using fs = std::filesystem;

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it can't be that, I'll say why below

@@ -11,11 +11,13 @@

using namespace nix;
Copy link
Member

Choose a reason for hiding this comment

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

nitpick(non-blocking): Would prefer to see this wrap the whole file.

@@ -11,11 +11,13 @@

using namespace nix;

namespace nix::fs { using namespace std::filesystem; }
Copy link
Member

Choose a reason for hiding this comment

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

I hadn't noticed this before, but there are a few of these nix::fs namespace declarations in src/nix now. I don't really understand what that's for? Just use std::filesystem so people don't have to wonder what nix::fs is.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's because of

namespace fs {
/**
* ```
* symlink_exists(p) = std::filesystem::exists(std::filesystem::symlink_status(p))
* ```
* Missing convenience analogous to
* ```
* std::filesystem::exists(p) = std::filesystem::exists(std::filesystem::status(p))
* ```
*/
inline bool symlink_exists(const std::filesystem::path & path) {
return std::filesystem::exists(std::filesystem::symlink_status(path));
}
} // namespace fs

I hope that with more std::filesystem, most of file-system.hh goes away. However, there are a few things that aren't in there they we need, like this symlink_exists.

I put it in an fs namespace with the idea that if C++ files (not headers!) use fs:: as a shorthand for std::filesystem::, then it will nicely gell with the existing apis.

namespace nix::fs { using namespace std::filesystem; }

I do because if I do

using fs = std::filesystem;

then that fs will shadow, rather than expand, the namespace fs I put in file-system.hh.

I tried a few combinations, and the thing you commented on is the only one that will work (other than declaring symlink_exists in std::filesystem directly, which would be ill-advised.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants