Conversation
aapoalas
force-pushed
the
temporal_intrinsic
branch
from
db6b7c0 to
5ceba18
Compare
lockels
force-pushed
the
temporal_intrinsic
branch
from
f9ced65 to
2d81989
Compare
…ntPrototype%Temporal.Instant.Prototype% as well as heap data and handles
…type.rs and instant_constructor.rs
aapoalas
force-pushed
the
temporal_intrinsic
branch
from
93a45c0 to
20d4473
Compare
plain_time constructor and prototype structs and stubs
…pe of the pull request
| weak-refs = [] | ||
| set = [] | ||
| typescript = [] | ||
| temporal = ["temporal_rs"] |
| /// an integer or a throw completion. | ||
| /// It converts argument to an integer representing its Number value, | ||
| /// or throws a RangeError when that value is not integral. | ||
| pub(crate) fn to_integer_if_integral<'gc>( |
There was a problem hiding this comment.
nitpick: Should be gated on the feature flag.
| if let Value::Integer(digits_value) = digits_value | ||
| && (0..=9).contains(&digits_value.into_i64()) | ||
| { | ||
| return Ok(temporal_rs::parsers::Precision::Digit( |
There was a problem hiding this comment.
nitpick: Maybe add a small comment regarding this fast path.
| // b. Return auto. | ||
| return Ok(temporal_rs::parsers::Precision::Auto); | ||
| } | ||
| // SAFETY: not shared. |
There was a problem hiding this comment.
issue: You should be throwing an error here. "If not 'auto', throw a RangeError exception. Else, return 'auto'."
| )); | ||
| } | ||
| // 3. If digitsValue is not a Number, then | ||
| if !digits_value.is_number() { |
There was a problem hiding this comment.
suggestion:
| if !digits_value.is_number() { | |
| let Ok(digits_value) = Number::try_from(digits_value) else { |
This should work to convert digitsValue into a number after this if branch; you just need to add the currently missing "throw RangeError" part inside the else.
| } | ||
| // 3. If digitsValue is not a Number, then | ||
| if !digits_value.is_number() { | ||
| let scoped_digits_value = digits_value.scope(agent, gc.nogc()); |
There was a problem hiding this comment.
note: This scoping becomes unnecessary.
| )); | ||
| } | ||
| // 5. Let digitCount be floor(ℝ(digitsValue)). | ||
| let digit_count = digits_value |
There was a problem hiding this comment.
issue: ToNumber here is extra. Spec's R(digitsValue) means effectively digits_value.to_real(agent) which is equivalent to into_f64.
|
|
||
| impl DurationHeapData<'_> { | ||
| pub fn default() -> Self { | ||
| todo!() |
There was a problem hiding this comment.
issue: This is either dead code or will crash the program when Temporal.Duration is used :)
There was a problem hiding this comment.
Should be fixed now, along with Duration constructor and AO's.
As a side note on Duration implementation, I've borrowed implementation from Instant.create_temporal_instant in:
fn create_temporal_duration
unsafe { object.set_duration(agent, duration) };where set_duration is defined as
/// # Safety
///
/// Should be only called once; reinitialising the value is not allowed.
unsafe fn set_duration(self, agent: &mut Agent, duration: temporal_rs::Duration) {
agent[self].duration = duration;
}Is this safe?
…oper default() for duration/data.rs
duration constructor, ao's and data.rs/default
| /// or a throw completion. It reads unit and rounding options needed by difference operations. | ||
| pub(crate) fn get_difference_settings<'gc, const IS_UNTIL: bool>( | ||
| agent: &mut Agent, | ||
| options: Object<'gc>, // options (an Object) |
There was a problem hiding this comment.
suggestion: No reason for the 'gc lifetime here.
| options: Object<'gc>, // options (an Object) | |
| options: Object, // options (an Object) |
| mut gc: GcScope<'gc, '_>, | ||
| ) -> JsResult<'gc, DifferenceSettings> { | ||
| let _unit_group = _unit_group.bind(gc.nogc()); | ||
| let _disallowed_units = _disallowed_units.bind(gc.nogc()); |
There was a problem hiding this comment.
note: Binding all of these arguments is entirely unnecessary since they do not have any lifetimes.
| @@ -0,0 +1,488 @@ | |||
| pub(crate) mod data; | |||
aapoalas
left a comment
aapoalas
left a comment
There was a problem hiding this comment.
Alright, lots of good code but also a lot of little corners to file and polish.
But overall, this is quite good as-is already, so excellent work! There weren't any big issues, no overt GC lifetime problems that I detected, or anything like that. Just nice, clean (or what passes as clean in Nova :) ) good code <3
| .unbind()? | ||
| .bind(gc.nogc()); | ||
| // 6. Perform ? ValidateTemporalUnitValue(largestUnit, unitGroup, « auto »). | ||
| // 7. If largestUnit is unset, then |
There was a problem hiding this comment.
question: Should these steps be added?
| gc.into_nogc(), | ||
| )); | ||
| }; | ||
| let Ok(new_target) = Function::try_from(new_target.unbind()) else { |
There was a problem hiding this comment.
note: Here you do not want to unbind new_target before calling try_from: if left bound then the Ok(new_target) would still be bound to the GC lifetime and would be safe. If you didn't scope the result into a variable shadowing this name, it'd be possible for someone to accidentally use new_target after free.
| .map_err(|e| temporal_err_to_js_err(agent, e, gc.nogc())) | ||
| .unbind()? | ||
| .bind(gc.nogc()); | ||
| create_temporal_duration(agent, duration.unbind(), Some(new_target.get(agent)), gc) |
There was a problem hiding this comment.
note: It shouldn't be necessary to unbind duration, as temporal_rs::Duration is just "plain old data" and does not carry the GC lifetime. In other words, types that are trivially_bindable! do not need unbind/bind at function call interfaces.
| }; | ||
| use temporal_rs::{TemporalError, error::ErrorKind}; | ||
|
|
||
| pub fn temporal_err_to_js_err<'gc>( |
There was a problem hiding this comment.
nitpick: pub(crate) should suffice.
| @@ -0,0 +1,33 @@ | |||
| use crate::{ | |||
There was a problem hiding this comment.
nitpick: Missing MPLv2 header.
| } | ||
|
|
||
| // 4. If roundTo is a String, then | ||
| let round_to = if round_to.unbind().is_string() { |
There was a problem hiding this comment.
note: Same thing here.
| let round_to = if round_to.unbind().is_string() { | |
| let round_to = if round_to.is_string() { |
| round_to | ||
| } else { | ||
| // 5. Else, set roundTo to ? GetOptionsObject(roundTo). | ||
| get_options_object(agent, round_to.unbind(), gc.nogc()) |
There was a problem hiding this comment.
Note: Unbinding round_to should also be unnecessary here; unbinding is only needed when passing references (that are properly bound) into calls that take a GcScope.
| .unbind()? | ||
| .bind(gc.nogc()); | ||
| // 19. Return ! CreateTemporalInstant(roundedNs). | ||
| Ok(create_temporal_instant(agent, rounded_ns, None, gc)?.into_value()) |
There was a problem hiding this comment.
note: This apparently cannot fail (! CreateTemporalInstant), so you can safely unwrap this. Or alternatively just use CreateHeapData directly.
| return Ok(RoundingIncrement::default()); | ||
| } | ||
| // 3. Let integerIncrement be ? ToIntegerWithTruncation(value). | ||
| let integer_increment = to_integer_with_truncation(agent, value, gc.reborrow()) |
There was a problem hiding this comment.
thought: I think the below test will always return the same result even if you do a direct as i64 conversion here: the f64 cannot be infinite or NaN, it can at most be a much-too-big integer which (I think) round to i64::MAX and i64::MIN. Those'll still be outside the valid increment range even if value isn't exactly the same as originally.
| dates: Vec::with_capacity(1024), | ||
| // TODO: assign appropriate value for Temporal objects. | ||
| #[cfg(feature = "temporal")] | ||
| instants: Vec::with_capacity(1024), |
3 participants
No description provided.