search

georust / geographiclib-rs

A port of geographiclib in Rust.
MIT License
40 stars 9 forks source link

reduce use of variable shadowing (geomath) #3

Open stonylohr opened 4 years ago

stonylohr commented 4 years ago

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.

michaelkirk commented 4 years ago

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

stonylohr commented 4 years ago

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.