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

Various refactorings #1470

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Various refactorings #1470

wants to merge 17 commits into from

Conversation

adamnemecek
Copy link
Contributor

  • made OMatrix::sum faster by using add_assign
  • use map on optionals where possible
  • refactored partial_cmp

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks, overall these are nice style improvements. For future reference, they'd be a lot easier to review if you grouped changes of the same kind into the same commit, and force-pushed when you discover an error rather than fixing it in a later commit.

src/base/ops.rs Show resolved Hide resolved
src/lib.rs Outdated
} else {
None
}
a.partial_cmp(b).map(|ord| if ord.is_ge() { b } else { a })
Copy link
Collaborator

Choose a reason for hiding this comment

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

To avoid a change in behavior, this should be is_gt (and similar below)

Copy link
Contributor Author

@adamnemecek adamnemecek Jan 5, 2025

Choose a reason for hiding this comment

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

This is the correct behavior, is_ge == matches!(self, Greater) which is what was there before.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's incorrect. The "e" stands for "equals", as in "greater or equals". This is illustrated in the documentation.

@adamnemecek adamnemecek changed the title Varios refactorings Various refactorings Jan 5, 2025
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