Should DAGNodeBuilder::then move the `work` away or just forward?

[kedixa]

The implementation of marl is very beautiful. I am learning new ideas from it, but I encountered a problem.

https://github.com/google/marl/blob/535d49182e6c87e4d999ac25f61c729a66687be8/include/marl/dag.h#L233-L239

In line 236, whether work is a reference or an rvalue reference, it is moved by std::move and used to create a new node. As shown in the example below, when I expected to use the same function to create multiple different nodes, my function was accidentally moved away. Is this a mistake or a feature? Is std::forward a better choice?

Example

#include <iostream>

#include "marl/dag.h"
#include "marl/scheduler.h"

void test_dag() {
    marl::DAG<void>::Builder builder;

    std::function<void()> func1([]() {
        std::cout << "hello" << std::endl;
    });

    std::function<void()> func2([]() {
        std::cout << "world" << std::endl;
    });

    auto root = builder.root();
    root.then(func1)
        .then(func2)
        .then(func1); // func1 is empty now

    auto dag = builder.build();
    dag->run();
}

int main() {
    marl::Scheduler::Config cfg;
    marl::Scheduler scheduler(cfg);
    scheduler.bind();

    test_dag();

    scheduler.unbind();
    return 0;
}

[amaiorano]

You're right, that should be std::forward, not a std::move. Would you be willing to create a fix pull request, along with a test like the one you wrote above?

[amaiorano]

Fixed: https://github.com/google/marl/commit/dbf097e43824d4b4ba45d5b696d86e147a4b9b00