Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reduce use of variable shadowing (geomath) #3

Open
stonylohr opened this issue Jul 15, 2020 · 2 comments
Open

reduce use of variable shadowing (geomath) #3

stonylohr opened this issue Jul 15, 2020 · 2 comments

Comments

@stonylohr
Copy link
Contributor

In rust, if you declare a variable with some name, and then later declare a variable with the same name in the same scope, it creates a second variable that hides or "shadows" the first one. See https://qvault.io/2020/05/13/variable-shadowing-in-rust-let-is-immutable-but-not-constant/ for one discussion of this topic.

While this technique can be useful, I don't believe it's desirable in any of the current uses in geomath. I propose removing all instances of this with equivalent mutable variables or arguments. Similar cases likely exist in other files, but I'm limiting this issue to just the geomath file for now, since it's the only one I've reviewed in depth.

Example 1 (shadowed variable):
// Current
pub fn sum(u: f64, v: f64) -> (f64, f64) {
//...
let vpp = s - up;
//...
let vpp = vpp - v; // Stony: shadows original variable vpp, rather than modifying it
//...
}
// Proposed
pub fn sum(u: f64, v: f64) -> (f64, f64) {
//...
let mut vpp = s - up;
//...
vpp -= v;
//...
}

Example 2 (shadowed argument):
// Current
pub fn polyval(n: i64, p: &[f64], s: usize, x: f64) -> f64 {
let mut s = s; // Stony: shadows argument s, creating an additional variable
let mut n = n; // Stony: shadows argument n, creating an additional variable
//...
}
//Proposed
pub fn polyval(mut n: i64, p: &[f64], mut s: usize, x: f64) -> f64 {
// (remove redeclarations, not replacing with anything)
//...
}

Note that I will later propose removing polyval's "s" argument entirely, in a separate issue.

This was referenced Jul 16, 2020
@michaelkirk
Copy link
Member

I'm not opposed. Is your thought that it will read more clearly?

@stonylohr
Copy link
Contributor Author

My primary thought is that it should provide a slight improvement in performance and memory usage, basically equivalent to removing an unused variable. I personally prefer non-shadowing from a style standpoint, but that may mostly be that I had previously expected that it would be a treated as a compile-time error. So it's hard to say how much of my preference is based on any real understanding of how idiomatic it is for rust.

This was referenced Jan 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants