search

Chia-Network / clvm_rs

Rust implementation of clvm
Apache License 2.0
66 stars 57 forks source link

[draft] [opinions welcome] Change the individual callback functions in run_program to a trait which simplifies lifetime management #398

Open prozacchiwawa opened 5 months ago

prozacchiwawa commented 5 months ago

This makes PreEval a trait which is passed down the stack as a mutable reference. It contains both pre_eval and post_eval methods and calls post_eval when requested based on the result of pre_eval as before.

Because the callback's lifetime is now easily determined to be the lifetime of the run_program call and the other object that could be of use, the allocator is also passed by a mutable reference, the caller has maximal freedom in how their implementation of PreEval works without having to use the Rc<RefCell<...>> pattern and cunning use of move closures to create the callbacks and avoids construction of a Box on each callback for the second closure.

Since this changes the form of the run_program interface, commentary on that is welcome. This is a response to the arch meeting discussion rather than something I'm actively requesting, but I do think this is better overall.

coveralls-official[bot] commented 5 months ago

Pull Request Test Coverage Report for Build 8850932024

Details

Totals Coverage Status
Change from base Build 8835105043: 0.0%
Covered Lines: 5789
Relevant Lines: 6162
💛 - Coveralls
prozacchiwawa commented 5 months ago

thanks @Rigidity for the typedef suggestion and catching the ungated declaration.

Rigidity commented 5 months ago

I think performance should still be a consideration, since features are additive - if you depend on clvm_tools_rs and clvmr in your project for instance, and clvm_tools_rs enables the feature, its performance effects will be seen on your usage of clvmr even if you don't specify the feature yourself. So we ideally wouldn't want it to be considerably slower, but I'm guessing the compiler can optimize this to around the same as before.

github-actions[bot] commented 3 months ago

'This PR has been flagged as stale due to no activity for over 60 days. It will not be automatically closed, but it has been given a stale-pr label and should be manually reviewed.'