-
Notifications
You must be signed in to change notification settings - Fork 43
Expose equals and compare over FFI #269
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
Changes from 1 commit
aab5e25
93a2c52
91ef5db
a6e4e39
54f2efd
4690d00
3f11fa9
932ca4c
64fe0d2
04d9d52
ffa2c71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -245,7 +245,71 @@ pub mod ffi { | |
| pub fn equals(&self, other: &Self) -> bool { | ||
| self.0 == other.0 | ||
| } | ||
|
|
||
|
|
||
| pub fn compare(one: &Self, two: &Self) -> i32 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I believe there should be a simpler way to implement these. The built-ins that have an The exception to this I believe is the time oriented types, which should implement |
||
| Self::compare_iso_plain_date_time( | ||
| one.iso_year(), | ||
| one.iso_month(), | ||
| one.iso_day(), | ||
| one.hour(), | ||
| one.minute(), | ||
| one.second(), | ||
| one.millisecond(), | ||
| one.microsecond(), | ||
| one.nanosecond(), | ||
| two.iso_year(), | ||
| two.iso_month(), | ||
| two.iso_day(), | ||
| two.hour(), | ||
| two.minute(), | ||
| two.second(), | ||
| two.millisecond(), | ||
| two.microsecond(), | ||
| two.nanosecond(), | ||
| ) | ||
| } | ||
|
|
||
| pub fn compare_iso_plain_date_time( | ||
| year1: i32, | ||
| month1: u8, | ||
| day1: u8, | ||
| hour1: u8, | ||
| minute1: u8, | ||
| second1: u8, | ||
| millisecond1: u16, | ||
| microsecond1: u16, | ||
| nanosecond1: u16, | ||
| year2: i32, | ||
| month2: u8, | ||
| day2: u8, | ||
| hour2: u8, | ||
| minute2: u8, | ||
| second2: u8, | ||
| millisecond2: u16, | ||
| microsecond2: u16, | ||
| nanosecond2: u16, | ||
| ) -> i32 { | ||
| let comparisons = [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: no, this should just use |
||
| year1.cmp(&year2), | ||
| month1.cmp(&month2), | ||
| day1.cmp(&day2), | ||
| hour1.cmp(&hour2), | ||
| minute1.cmp(&minute2), | ||
| second1.cmp(&second2), | ||
| millisecond1.cmp(&millisecond2), | ||
| microsecond1.cmp(µsecond2), | ||
| nanosecond1.cmp(&nanosecond2), | ||
| ]; | ||
| comparisons | ||
| .iter() | ||
| .find(|&&ord| ord != std::cmp::Ordering::Equal) | ||
| .map_or(0, |ord| match ord { | ||
| std::cmp::Ordering::Greater => 1, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooooooooh, good to know! |
||
| std::cmp::Ordering::Less => -1, | ||
| std::cmp::Ordering::Equal => 0, | ||
| }) | ||
| } | ||
|
|
||
| pub fn round(&self, options: RoundingOptions) -> Result<Box<Self>, TemporalError> { | ||
| self.0 | ||
| .round(options.try_into()?) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ pub mod ffi { | |
| use crate::plain_date::ffi::{PartialDate, PlainDate}; | ||
|
|
||
| use diplomat_runtime::DiplomatWrite; | ||
| use std::cmp::Ordering; | ||
| use std::fmt::Write; | ||
|
|
||
| #[diplomat::opaque] | ||
|
|
@@ -47,7 +48,30 @@ pub mod ffi { | |
| pub fn equals(&self, other: &Self) -> bool { | ||
| self.0 == other.0 | ||
| } | ||
|
|
||
|
|
||
| pub fn compare(one: &Self, two: &Self) -> i32 { | ||
| Self::compare_iso_month_day( | ||
| one.iso_month(), | ||
| one.iso_day(), | ||
| two.iso_month(), | ||
| two.iso_day(), | ||
| ) | ||
| } | ||
|
|
||
| /// Compares two ISO month-day pairs and returns -1, 0, or 1. | ||
| pub fn compare_iso_month_day(month1: u8, day1: u8, month2: u8, day2: u8) -> i32 { | ||
| if month1 > month2 { | ||
| 1 | ||
| } else if month1 < month2 { | ||
| -1 | ||
| } else if day1 > day2 { | ||
| 1 | ||
| } else if day1 < day2 { | ||
| -1 | ||
| } else { | ||
| 0 | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue: PlainMonthDay does not have a compare function |
||
| } | ||
| pub fn iso_year(&self) -> i32 { | ||
| self.0.iso_year() | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,6 +142,28 @@ pub mod ffi { | |
| pub fn equals(&self, other: &Self) -> bool { | ||
| self.0 == other.0 | ||
| } | ||
| pub fn compare(one: &Self, two: &Self) -> i32 { | ||
| Self::compare_iso_year_month( | ||
| one.iso_year(), | ||
| one.iso_month(), | ||
| two.iso_year(), | ||
| two.iso_month(), | ||
| ) | ||
| } | ||
|
|
||
| pub fn compare_iso_year_month(year1: i32, month1: u8, year2: i32, month2: u8) -> i32 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: write this as a match on And link to the spec entry
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, thought: I don't think it would be a bad idea to just have a method return std::cmp::Ordering here, that converts to the integer anyway, and makes it easier to add comparator support later
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should i do this for all the new compare funcitons? |
||
| if year1 > year2 { | ||
| 1 | ||
| } else if year1 < year2 { | ||
| -1 | ||
| } else if month1 > month2 { | ||
| 1 | ||
| } else if month1 < month2 { | ||
| -1 | ||
| } else { | ||
| 0 | ||
| } | ||
| } | ||
| pub fn to_plain_date(&self) -> Result<Box<PlainDate>, TemporalError> { | ||
| self.0 | ||
| .to_plain_date() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the library's compare function. Don't write your own. only write a thing wrapper.