Skip to content

Mont 64#1

Open
Al-Kindi-0 wants to merge 15 commits into
mainfrom
mont_64
Open

Mont 64#1
Al-Kindi-0 wants to merge 15 commits into
mainfrom
mont_64

Conversation

@Al-Kindi-0

Copy link
Copy Markdown
Owner

First PR

@Al-Kindi-0 Al-Kindi-0 requested a review from bobbinth June 13, 2022 21:20

@bobbinth bobbinth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks good. I left a bunch of small comments inline.

Comment thread math/Cargo.toml
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs
Comment thread math/src/field/f64/mod.rs
Comment thread math/src/field/f64/mod.rs
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated

@bobbinth bobbinth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left a few more nits inline.

One other thing - let's move msquare() and equals() into helper function sections. Unless there is a need to expose them in the public interface.

Comment thread math/Cargo.toml Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated

@bobbinth bobbinth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I left a couple of minor nits inline.

I think the last thing left is to make sure serialization tests pass as well.

Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated
Comment thread math/src/field/f64/mod.rs Outdated

@bobbinth bobbinth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I left a few small nits inline. Also, I think a comment on line 18 needs to be updated.

Comment thread math/src/field/f64/tests.rs Outdated
Comment thread math/src/field/f64/tests.rs Outdated
Comment thread math/src/field/f64/tests.rs Outdated
Comment thread math/src/field/f64/tests.rs Outdated
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

Successfully merging this pull request may close these issues.

2 participants