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

address issues 10 #15

Merged
merged 4 commits into from Jul 24, 2019
Merged

address issues 10 #15

merged 4 commits into from Jul 24, 2019

Conversation

@HAOYUatHZ HAOYUatHZ changed the title address https://github.com/KZen-networks/class-groups/issues/10 address issues 10 Jul 21, 2019
Copy link
Member

amanusk left a comment

This patch failed to build for me, I suggested some minor change :)

src/lib.rs Outdated
pub fn mul(&self, another: Self) -> Self {
Matrix22 {
a11: self.a11 * &another.a11 + self.a12 * &another.a21,
a12: self.a11 * &another.a12 + self.a12 * &another.a22,

This comment has been minimized.

Copy link
@amanusk

amanusk Jul 22, 2019

Member

This change is causing the build to fail for me.
Errors:

cannot move out of `self.a11` which is behind a shared reference                                                               
  --> src/lib.rs:52:18                                                                                                                       
   |                                                                                                                                                           
52 |             a11: self.a11 * &another.a11 + self.a12 * &another.a21,                                                                                       
   |                  ^^^^^^^^ move occurs because `self.a11` has type `gmp::mpz::Mpz`, which does not implement the `Copy` trait      

I suggest updating this to:

        Matrix22 {
            a11: &self.a11 * &another.a11 + &self.a12 * &another.a21,
            a12: &self.a11 * &another.a12 + &self.a12 * &another.a22,
            a21: &self.a21 * &another.a11 + &self.a22 * &another.a21,
            a22: &self.a21 * &another.a12 + &self.a22 * &another.a22,
        }
src/lib.rs Outdated
@@ -46,6 +46,17 @@ pub struct Matrix22 {
pub a22: BigInt,
}

impl Matrix22 {
pub fn mul(&self, another: Self) -> Self {

This comment has been minimized.

Copy link
@amanusk

amanusk Jul 22, 2019

Member

It could be a good idea to only borrow another with another: &Self, in case it needs to be used more than once

@HAOYUatHZ

This comment has been minimized.

Copy link
Contributor Author

HAOYUatHZ commented Jul 23, 2019

@amanusk i have fixed my PR according to ur comments, would u like to take a look again ?

couldn't find the "re-request review from" button on github

@amanusk

This comment has been minimized.

Copy link
Member

amanusk commented Jul 23, 2019

@HAOYUatHZ, thanks, this still does not compile:

rror[E0204]: the trait `Copy` may not be implemented for this type
  --> src/lib.rs:42:10
   |
42 | #[derive(Copy, Clone)]
   |          ^^^^
43 | pub struct Matrix22 {
44 |     pub a11: BigInt,
   |     --------------- this field does not implement `Copy`
45 |     pub a12: BigInt,
   |     --------------- this field does not implement `Copy`
46 |     pub a21: BigInt,
   |     --------------- this field does not implement `Copy`
47 |     pub a22: BigInt,
   |     --------------- this field does not implement `Copy`

The problem is the Copy trait of BigInt not Matrix22

@HAOYUatHZ

This comment has been minimized.

Copy link
Contributor Author

HAOYUatHZ commented Jul 24, 2019

@HAOYUatHZ, thanks, this still does not compile:

rror[E0204]: the trait `Copy` may not be implemented for this type
  --> src/lib.rs:42:10
   |
42 | #[derive(Copy, Clone)]
   |          ^^^^
43 | pub struct Matrix22 {
44 |     pub a11: BigInt,
   |     --------------- this field does not implement `Copy`
45 |     pub a12: BigInt,
   |     --------------- this field does not implement `Copy`
46 |     pub a21: BigInt,
   |     --------------- this field does not implement `Copy`
47 |     pub a22: BigInt,
   |     --------------- this field does not implement `Copy`

The problem is the Copy trait of BigInt not Matrix22

i see
was using i32 for testing my prototype code

sorry for that

@HAOYUatHZ

This comment has been minimized.

Copy link
Contributor Author

HAOYUatHZ commented Jul 24, 2019

fixed. can cargo build & pass cargo test by using ur @amanusk #18

@amanusk

This comment has been minimized.

Copy link
Member

amanusk commented Jul 24, 2019

Looks good to me!

@omershlo omershlo merged commit deb6d7c into KZen-networks:master Jul 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.