clarkmcc / cel-rust

Common Expression Language interpreter written in Rust
https://crates.io/crates/cel-interpreter
MIT License
362 stars 18 forks source link

Get variable and function references from expression #61

Closed clarkmcc closed 2 months ago

clarkmcc commented 2 months ago

Closes #60

kameko commented 2 months ago

One thing I would note in the doc comments in ast.rs is that the variables and functions methods in ExpressionReferences clone their internal HashSets, for anyone being memory conscious. This is a practice I picked up in my own code as a courtesy. I wouldn't bother with making it non-clone for now since that can be resolved in a future PR if it's absolutely necessary.

clarkmcc commented 2 months ago

So I'm assuming you're talking about this

pub fn variables(&self) -> Vec<&str> {
    self.variables.iter().cloned().collect()
}

But that doesn't clone the iterator, it just clones each element which in this case is &&str so we're just dereferencing pointers to &str (i.e. we get an owned T, which for &&str is &str, sorta weird). Because we're borrowing the variables iterator we get references to &str (&&str) which results in &str.

I double-checked my work on this and found that there's actually a .copied() method. In this case it's effectively the same, but I think it more clearly shows what's actually happening so I'll change it to that.

let a: Vec<&&str> = self.variables.iter().collect();
let b: Vec<&str> = self.variables.iter().cloned().collect();
let c: Vec<&str> = self.variables.iter().copied().collect();
kameko commented 2 months ago

Oh! I didn't even notice, I just took a brief overview of the new API in GitHub (so no type hints). Never mind then!