Skip to content

types: remove Path* typedefs#15354

Merged
Ericson2314 merged 1 commit intoNixOS:masterfrom
obsidiansystems:convert-path-setting
Feb 27, 2026
Merged

types: remove Path* typedefs#15354
Ericson2314 merged 1 commit intoNixOS:masterfrom
obsidiansystems:convert-path-setting

Conversation

@amaanq
Copy link
Member

@amaanq amaanq commented Feb 26, 2026

Motivation

We want to get rid of the unnecessary Path* typedefs. This commit replaces all usages with their underlying types, that being std::string for store paths, std::filesystem::path for filesystem paths, StringSet for path sets, and std::string_view` for non-owning references.


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 new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking c api Nix as a C library with a stable interface labels Feb 26, 2026
@amaanq amaanq force-pushed the convert-path-setting branch from 0a1be0d to f10346c Compare February 26, 2026 20:18
@amaanq amaanq force-pushed the convert-path-setting branch from f10346c to 3f45105 Compare February 26, 2026 20:57
@amaanq amaanq force-pushed the convert-path-setting branch 2 times, most recently from de4aa1b to 7a4bd97 Compare February 26, 2026 22:00
@amaanq amaanq mentioned this pull request Feb 26, 2026
@amaanq amaanq force-pushed the convert-path-setting branch 6 times, most recently from cabbcd8 to a044829 Compare February 27, 2026 04:06
@amaanq amaanq force-pushed the convert-path-setting branch from a044829 to 4a8451b Compare February 27, 2026 05:51
This commit replaces all usages with their underlying types, that being `std::string` for store
paths, `std::filesystem::path` for filesystem paths, `StringSet` for
path sets, and `std::string_view` for non-owning references.
@amaanq amaanq force-pushed the convert-path-setting branch from 4a8451b to fc08c86 Compare February 27, 2026 05:55
@Ericson2314 Ericson2314 added this pull request to the merge queue Feb 27, 2026
Merged via the queue into NixOS:master with commit b0c932d Feb 27, 2026
15 checks passed
@Ericson2314 Ericson2314 deleted the convert-path-setting branch February 27, 2026 07:22
Comment on lines +225 to +230
struct AbsolutePath : std::filesystem::path
{
using path::path;
using path::operator=;

AbsolutePath(const std::filesystem::path & p)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be careful to avoid slicing here. Maybe a better pattern would be private inheritance and using all necessary methods?

Copy link
Member

Choose a reason for hiding this comment

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

It could just be a wrapper type with a single std::filesystem::path field. That is easy to reason about for any C++ noobs :).

return bd.has_value() ? *bd
: buildDir.get().has_value() ? *buildDir.get()
: std::filesystem::path{stateDir.get()} / "builds";
: AbsolutePath{stateDir.get() / "builds"};
Copy link
Contributor

Choose a reason for hiding this comment

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

This does slicing, not good

Copy link
Member

Choose a reason for hiding this comment

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

Wrapper type would instead have it take ownership, seems good.

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

Labels

c api Nix as a C library with a stable interface fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants