clvm_rs icon indicating copy to clipboard operation
clvm_rs copied to clipboard

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

Open prozacchiwawa opened this issue 9 months ago • 4 comments

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.

prozacchiwawa avatar Apr 25 '24 19:04 prozacchiwawa

Pull Request Test Coverage Report for Build 8850932024

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 93.947%

Totals Coverage Status
Change from base Build 8835105043: 0.0%
Covered Lines: 5789
Relevant Lines: 6162

💛 - Coveralls

coveralls-official[bot] avatar Apr 25 '24 19:04 coveralls-official[bot]

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

prozacchiwawa avatar Apr 25 '24 20:04 prozacchiwawa

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.

Rigidity avatar Apr 25 '24 23:04 Rigidity

'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.'

github-actions[bot] avatar Jun 26 '24 11:06 github-actions[bot]