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

Add is_one. #39

Merged
merged 3 commits into from
Feb 23, 2018
Merged

Add is_one. #39

merged 3 commits into from
Feb 23, 2018

Conversation

clarfonthey
Copy link
Contributor

Implements the version recommended in #5. That issue should remain open to track the breaking-change version.

/// Returns `true` if `self` is equal to the multiplicative identity.
///
/// Compatibility note: this method will be a requirement to implement in the future; new
/// implementors should provide it for future-compatibility.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little hesitant about this scary-sounding note, because we're definitely not going to change that without a semver bump! And when we do, it should also drop the Self: PartialEq, which means they'll have porting work to do anyway. Maybe instead just advise that implementations should consider adding it now for better performance, noting that it will only become required in a future breaking change.

Also, let's put #[inline] on this.

@clarfonthey
Copy link
Contributor Author

Applied your suggestions; is the caveat better now?

@cuviper
Copy link
Member

cuviper commented Feb 23, 2018

Works for me, thanks!

bors r+

bors bot added a commit that referenced this pull request Feb 23, 2018
39: Add is_one. r=cuviper a=clarcharr

Implements the version recommended in #5. That issue should remain open to track the breaking-change version.
@bors
Copy link
Contributor

bors bot commented Feb 23, 2018

Build succeeded

@bors bors bot merged commit 51dad50 into rust-num:master Feb 23, 2018
@clarfonthey clarfonthey deleted the is_one branch February 27, 2018 18:28
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