You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
The text was updated successfully, but these errors were encountered:
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.
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.
The text was updated successfully, but these errors were encountered: