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

Reconsider Raw pattern #7479

Open
fricklerhandwerk opened this issue Dec 19, 2022 · 5 comments
Open

Reconsider Raw pattern #7479

fricklerhandwerk opened this issue Dec 19, 2022 · 5 comments
Assignees
Labels
contributor-experience Developer experience for Nix contributors idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.

Comments

@fricklerhandwerk
Copy link
Contributor

We discussed in Nix team meeting on 2022-12-19, that there are multiple instances in the code which follow this pattern:

typedef std::variant<AllOutputs, OutputNames> _OutputsSpecRaw;

struct OutputsSpec : _OutputsSpecRaw {
    using Raw = _OutputsSpecRaw;
    using Raw::Raw;

    using Names = OutputNames;
    using All = AllOutputs;

    inline const Raw & raw() const {
        return static_cast<const Raw &>(*this);
    }

    inline Raw & raw() {
        return static_cast<Raw &>(*this);
    }

@edolstra asks whether we should do it that way, because it's quite verbose. Also adding aliases like in this case keeps both names around in the codebase.

@Ericson2314 do we have alternatives that are as convenient to use at the call site? Does there speak anything against having the variants as fields?

@fricklerhandwerk fricklerhandwerk added the contributor-experience Developer experience for Nix contributors label Dec 19, 2022
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2022-12-19-nix-team-meeting-minutes-18/24121/1

@Ericson2314
Copy link
Member

My thoughts on the raw pattern are basically:

  1. Yes, the declaration is ugly and verbose, but it makes the code that uses the definition much nicer to read, and that is more important than the definition itself being nice to read.

  2. When we someday rewrite in Rust this problem will go away, as Rust supports exactly this sort of code and data definition very nicely.

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Mar 30, 2023
We weren't because this ancient PR predated it!

This is actually a new version of the pattern which addresses some
issues identified in NixOS#7479.
@Ericson2314
Copy link
Member

My friend @jhartzell42 taught me about std::forward, which yields us a better version of this pattern!

The example in the issue would be rewritten:

struct OutputsSpec {
    struct Names { ... };

    struct All { ... };

    typedef std::variant<AllOutputs, OutputNames> Raw;

    Raw raw;

    /* The moral equivalent of `using Raw::Raw;` */
    OutputsSpec(auto &&... arg)
        : raw(std::forward<decltype(arg)>(arg)...)
    { }

Note how there are no more bad ...Raw names littering the top level!

I have given this new version a spin in c51d554; it is working well. I hope we can convert the other sum types in the codebase soon!

@Ericson2314
Copy link
Member

@tfc By getting rid of inheritance, I hope this address your concerns from #6815 also!

@Ericson2314 Ericson2314 added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Apr 3, 2023
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-04-03-nix-team-meeting-minutes-46/27008/1

Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Aug 16, 2023
See that issue for details.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Aug 16, 2023
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Aug 16, 2023
See that issue for details on the general goal.

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Aug 18, 2023
Types converted:

- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.

See that issue (NixOS#7479) for details on the general goal.

`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Aug 18, 2023
Types converted:

- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.

See that issue (NixOS#7479) for details on the general goal.

`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue Aug 18, 2023
Types converted:

- `NixStringContextElem`
- `OutputsSpec`
- `ExtendedOutputsSpec`
- `DerivationOutput`
- `DerivationType`

Existing ones mostly conforming the pattern cleaned up:

- `ContentAddressMethod`
- `ContentAddressWithReferences`

The `DerivationGoal::derivationType` field had a bogus initialization,
now caught, so I made it `std::optional`. I think NixOS#8829 can make it
non-optional again because it will ensure we always have the derivation
when we construct a `DerivationGoal`.

See that issue (NixOS#7479) for details on the general goal.

`git grep 'Raw::Raw'` indicates the two types I didn't yet convert
`DerivedPath` and `BuiltPath` (and their `Single` variants) . This is
because @roberth and I (can't find issue right now...) plan on reworking
them somewhat, so I didn't want to churn them more just yet.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>
Ericson2314 added a commit that referenced this issue Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.
Projects
None yet
Development

No branches or pull requests

3 participants