Split up `primops.cc`

rbt commented

2024-03-14 23:12:00 +00:00

Owner

Nearly every primop is defined in one file. Move them into different files.

jade added the

devx

label 2024-03-15 20:29:36 +00:00

tom-hubrecht commented

2024-05-29 11:56:28 +00:00

Member

I'm starting to tackle this, one question I have, some primops are define with lambda funtions :

static RegisterPrimOp primop_throw({
    .name = "throw",
    .args = {"s"},
    .doc = R"(
      Throw an error message *s*. This usually aborts Nix expression
      evaluation, but in `nix-env -qa` and other commands that try to
      evaluate a set of derivations to get information about those
      derivations, a derivation that throws an error is silently skipped
      (which is not the case for `abort`).
    )",
    .fun = [](EvalState & state, const PosIdx pos, Value * * args, Value & v)
    {
      NixStringContext context;
      auto s = state.coerceToString(pos, *args[0], context,
              "while evaluating the error message passed to builtin.throw").toOwned();
      state.error<ThrownError>(s).debugThrow();
    }
});

but other define a prim_* function, where this function is not used anywhere else :

static void prim_ceil(EvalState & state, const PosIdx pos, Value * * args, Value & v)
{
    auto value = state.forceFloat(*args[0], args[0]->determinePos(pos),
            "while evaluating the first argument passed to builtins.ceil");
    v.mkInt(ceil(value));
}

static RegisterPrimOp primop_ceil({
    .name = "__ceil",
    .args = {"double"},
    .doc = R"(
        Converts an IEEE-754 double-precision floating-point number (*double*) to
        the next higher integer.

        If the datatype is neither an integer nor a "float", an evaluation error will be
        thrown.
    )",
    .fun = prim_ceil,
});

It would be great to have a uniform way to define the primops, which one should we choose ?

I'm starting to tackle this, one question I have, some primops are define with lambda funtions : ```cpp static RegisterPrimOp primop_throw({ .name = "throw", .args = {"s"}, .doc = R"( Throw an error message *s*. This usually aborts Nix expression evaluation, but in `nix-env -qa` and other commands that try to evaluate a set of derivations to get information about those derivations, a derivation that throws an error is silently skipped (which is not the case for `abort`). )", .fun = [](EvalState & state, const PosIdx pos, Value * * args, Value & v) { NixStringContext context; auto s = state.coerceToString(pos, *args[0], context, "while evaluating the error message passed to builtin.throw").toOwned(); state.error<ThrownError>(s).debugThrow(); } }); ``` but other define a `prim_*` function, where this function is not used anywhere else : ```cpp static void prim_ceil(EvalState & state, const PosIdx pos, Value * * args, Value & v) { auto value = state.forceFloat(*args[0], args[0]->determinePos(pos), "while evaluating the first argument passed to builtins.ceil"); v.mkInt(ceil(value)); } static RegisterPrimOp primop_ceil({ .name = "__ceil", .args = {"double"}, .doc = R"( Converts an IEEE-754 double-precision floating-point number (*double*) to the next higher integer. If the datatype is neither an integer nor a "float", an evaluation error will be thrown. )", .fun = prim_ceil, }); ``` It would be great to have a uniform way to define the primops, which one should we choose ?

delroth commented

2024-05-29 12:52:21 +00:00

Member

Semi orthogonal, but to keep in mind while doing this refactoring: #359

Semi orthogonal, but to keep in mind while doing this refactoring: https://git.lix.systems/lix-project/lix/issues/359

delroth commented

2024-05-29 12:55:22 +00:00

Member

(Also: of your two options, I personally slightly prefer option number 1. There might be exceptions where I find number 2 better though, e.g. when the implementation of the primop is complex and long.)

tom-hubrecht commented

2024-05-29 14:17:01 +00:00

Member

Semi orthogonal, but to keep in mind while doing this refactoring: #359

Right, I have an idea on how to do it but not sure how to implement it.

Each file toto.cc in src/libexpr/primops defines a PrimOp struct primop_toto and in primops.cc we have the function EvalState::createBaseEnv which will call addPrimop(primop_toto) for each primop.

The complicated part I guess is to automatically extract the list of primops to initialize, or we can do it by hand, given that we don't add primops every week.

> Semi orthogonal, but to keep in mind while doing this refactoring: https://git.lix.systems/lix-project/lix/issues/359 Right, I have an idea on how to do it but not sure how to implement it. Each file `toto.cc` in `src/libexpr/primops` defines a `PrimOp` struct `primop_toto` and in `primops.cc` we have the function `EvalState::createBaseEnv` which will call `addPrimop(primop_toto)` for each primop. The complicated part I guess is to automatically extract the list of primops to initialize, or we can do it by hand, given that we don't add primops every week.

delroth commented

2024-05-29 14:18:22 +00:00

Member

Yeah, I'd tend towards doing it by hand too, and the approach you're describing is what I was thinking about too. FWIW I'm not suggesting you have to take care of #359 along the way, but if you want to help that would be welcome!

tom-hubrecht commented

2024-05-29 14:21:30 +00:00

Member

I'll do it in the final commit of the chain I think, first I need to split everything ^^

pennae commented

2024-05-29 14:22:35 +00:00

Owner

(Also: of your two options, I personally slightly prefer option number 1. There might be exceptions where I find number 2 better though, e.g. when the implementation of the primop is complex and long.)

we'll disagree and prefer option number 2 (explicit implentation functions) for reasons of indentation stacking being less bad in that case and clang-format still sometimes misformatting lambdas. for small primops like in the example it'll be absolutely fine, but a lot of them unfortunately aren't quite that tiny. proposed heuristic: if you want to add newlines to group things for clarity, make it a static function instead of a lambda?

> (Also: of your two options, I personally slightly prefer option number 1. There might be exceptions where I find number 2 better though, e.g. when the implementation of the primop is complex and long.) we'll disagree and prefer option number 2 (explicit implentation functions) for reasons of indentation stacking being less bad in that case and clang-format still sometimes misformatting lambdas. for small primops like in the example it'll be absolutely fine, but a lot of them unfortunately aren't quite that tiny. proposed heuristic: if you want to add newlines to group things for clarity, make it a static function instead of a lambda?

pennae commented

2024-05-29 20:54:31 +00:00

Owner

I'll do it in the final commit of the chain I think, first I need to split everything ^^

unfortunately in your current attempt of one primop per file compile times have gone through the roof, +25% for splitting that one file. we can't in good conscience approve that, compile times are already far too long :/ maybe running iwyu over the final splits will undo this, but until we can find a way to undo it we'd rather have one file per topic (type support, interop formats, attrset functions, list functions, etc) :(

> I'll do it in the final commit of the chain I think, first I need to split everything ^^ unfortunately in your current attempt of one primop per file compile times have gone through the roof, +25% for splitting that one file. we can't in good conscience approve that, compile times are already far too long :/ maybe running iwyu over the final splits will undo this, but until we can find a way to undo it we'd rather have one file per topic (type support, interop formats, attrset functions, list functions, etc) :(

👀 1

tom-hubrecht commented

2024-05-29 21:57:54 +00:00

Member

we'd rather have one file per topic (type support, interop formats, attrset functions, list functions, etc) :(

Soooo, do I need to rewrite the whole chain or can I fuse some files at the end of the chain ?

> we'd rather have one file per topic (type support, interop formats, attrset functions, list functions, etc) :( Soooo, do I need to rewrite the whole chain or can I fuse some files at the end of the chain ?

pennae commented

2024-05-29 22:05:21 +00:00

Owner

that depends. if you're going to rebase the chain we'd kindly ask you to abandon and reorder+squash instead, if you're not going to rebase fusing at the end should be fine. but we should absolutely avoid another DoS situation on ci :)

Split up primops.cc #42

Split up `primops.cc` #42