Chia-Network / clvm_rs

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

`get_args`, and drop `Node` #315

Closed arvidn closed 1 year ago

arvidn commented 1 year ago

This change moves away from Node as the abstraction to iterate over lists, replacing it with two lower level facilities:

Some utility functions (in op_utils.rs) were changed to take an &Allocator + NodePtr rather than &Node as well, to allow dropping the Node class.

The get_args() abstraction is a quite nice improvement to the ergonomics of the operator implementations, and in my mind is the main motiviation for this patch.

I added a few missing unit tests for the updated utility functions as well as Allocator::atom_len().

The original intention of removing the Node abstraction was to allow for Allocator references to be mutable, and have Allocator::atom() (and related functions) take a mutable reference to the allocator. However, some experimentation beyond this point suggests that's actually impractical. But I think this step is still an improvement.

coveralls-official[bot] commented 1 year ago

Pull Request Test Coverage Report for Build 5294089619


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/run_program.rs 22 23 95.65%
src/more_ops.rs 100 108 92.59%
<!-- Total: 433 442 97.96% -->
Files with Coverage Reduction New Missed Lines %
src/run_program.rs 1 97.52%
<!-- Total: 1 -->
Totals Coverage Status
Change from base Build 5294056862: 0.08%
Covered Lines: 5138
Relevant Lines: 5525

💛 - Coveralls
arvidn commented 1 year ago

I'll address these comments in a separate PR. I hope that's OK.

This message isn't quite right. The idea it's trying to get across is that X must be an atom and not a cons. It can take multiple arguments (which are implicitly quoted).

Yeah, I agree that I was a bit lazy in making that use of get_args() produce a good error message. I'd prefer to keep get_args() relatively simple, and not make the error message it produces configurable (because it's good in almost all cases). I could make it say: in the ((X)...) syntax, the inner list takes exactly 1 argument

This might work (and cargo fmt will clean it up better):

string literals don't automatically concatenate in rust, but you can insert line breaks using \.

((+) 1 2 3) = 6 We should make sure there is at least one test of this.

There is at least one positive test for this construct, I'll add this example as well.

arvidn commented 1 year ago

https://github.com/Chia-Network/clvm_rs/pull/316