Skip to content

Conversation

@jturner314
Copy link
Member

The implementation of arr1 (op) &arr2 mutates arr1 for efficiency, but this caused surprising behavior if arr1 was ArrayViewMut. This commit requires that arr own its data to avoid surprising side effects.

This is an example of the old behavior that was surprising:

extern crate ndarray;

fn main() {
    let mut a = array![1., 2.];
    let b = array![3., 4.];

    // Prints a = [1, 2].
    println!("a = {}", a);

    {
        let a_view_mut = a.view_mut();
        // Prints a + b = [4, 6].
        println!("a + b = {}", a_view_mut + &b);
    }

    // Prints a = [4, 6]; would expect a = [1, 2].
    println!("a = {}", a);
}

Note that while this is a breaking change, the stricter trait bound is actually the correct bound according to the documentation.

The implementation of `arr1 (op) &arr2` mutates `arr1` for efficiency,
but this caused surprising behavior if `arr1` was `ArrayViewMut`. This
commit requires that `arr` own its data to avoid surprising side
effects.

This is an example of the old behavior that was surprising:

```rust
extern crate ndarray;

fn main() {
    let mut a = array![1., 2.];
    let b = array![3., 4.];

    // Prints a = [1, 2].
    println!("a = {}", a);

    {
        let a_view_mut = a.view_mut();
        // Prints a + b = [4, 6].
        println!("a + b = {}", a_view_mut + &b);
    }

    // Prints a = [4, 6]; would expect a = [1, 2].
    println!("a = {}", a);
}
```

Note that while this is a breaking change, the stricter trait bound is
actually the correct bound according to the documentation.
@bluss
Copy link
Member

bluss commented Nov 3, 2017

Right, we have probably fixed this already for the other ops.

@bluss
Copy link
Member

bluss commented Nov 3, 2017

Thanks

@bluss
Copy link
Member

bluss commented Nov 3, 2017

Previous pr #93

Unrelated pr, but something we talked about today was #103

@bluss bluss merged commit 5802f00 into rust-ndarray:master Nov 20, 2017
@jturner314 jturner314 deleted the fix-arith-mut branch November 20, 2017 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants