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

Time Args support for month string values #2413

Merged
merged 7 commits into from
Feb 19, 2023
Merged

Conversation

b-n
Copy link
Member

@b-n b-n commented Feb 18, 2023

Another follow on from #2410 and #2411.
Related to #2223

Time#utc, Time#local, and friends all support specifying a month value from the shortened 3 character english names for the month.

Ref: https://ruby-doc.org/3.1.2/Time.html#method-c-utc

@b-n b-n requested a review from lopopolo February 18, 2023 12:32
Time#utc, Time#local, and friends all support specifying a month value
from the shortened 3 character english names for the month.

Ref: https://ruby-doc.org/3.1.2/Time.html#method-c-utc
Copy link
Member

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

love that we're getting so close to parity here. I find the structure of the if let hard to follow. It reminds me of this pattern in golang which I also find hard to read:

if err := func() {
    // do
    // the
    // thing
    return err
    // do
    // do
    // do
    return nil
}(); err != nil {
    return nil
}

I think the simplest way to do the case insensitivity is with an allocation to Vec<u8> combined with BStr::make_ascii_lowercase. Maybe we can short circuit if the length of the byteslice != 3?

artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
// => 2
// ```
let month: i64 = if let Ok(month) = to_str(self, arg).and_then(|arg| {
let month_str: &str = arg.try_convert_into_mut(self)?;
Copy link
Member

Choose a reason for hiding this comment

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

since we're checking against an ASCII allowlist, we can use &[u8] and b"jan" byte literals here to avoid a UTF-8 decode.

Copy link
Member Author

Choose a reason for hiding this comment

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

make_ascii_lowercase() - which nicely does convert in place - needed a &mut [u8] conversion for Artichoke (which doesn't exist currently). I assume that's for a reason rather than adding my own - so the only way this was possible was to make it convert to a Vec, and do the comparison of byte slices with as_slice().

Is there any particular reason there isn't a conversion to &mut [u8] as of yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

Note:

impl<'a> TryConvertMut<Value, &'a mut [u8]> for Artichoke {
    type Error = Error;

    fn try_convert_mut(&mut self, mut value: Value) -> Result<&'a mut [u8], Self::Error> {
        self.protect(value);
        let mut s = unsafe { String::unbox_from_value(&mut value, self)? };
        // SAFETY: This transmute modifies the lifetime of the byte slice pulled
        // out of the boxed `String`. This requires that no garbage collections
        // that reclaim `value` occur while this slice is alive. This is
        // enforced for at least this entry from an mruby trampoline by the call
        // to `protect` above.
        //
        // FIXME: does this unbound lifetime and transmute below allow
        // extracting `&'static [u8]`?
        let slice = unsafe { mem::transmute(s.as_inner_mut().as_mut_slice()) };
        Ok(slice)
    }
}

appears to be "valid rust". I am very okay in admitting that I'm not entirely sure if this makes 100% rust/memory sense (still a topic I need to get a properly deep understanding of)

artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Show resolved Hide resolved
A couple of things I missed is that failure to match on string
conversion results in a call to `Kernel#Integer`, thus the new import.

Additionally, month value matches are now case insensitive and match on
byte strings to avoid addition UTF-8 processing in rust.
@b-n b-n requested a review from lopopolo February 19, 2023 10:21
Copy link
Member

@lopopolo lopopolo left a comment

Choose a reason for hiding this comment

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

I left some style suggestions which you don't need to take. Logic LGTM.

artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Outdated Show resolved Hide resolved
artichoke-backend/src/extn/core/time/args.rs Show resolved Hide resolved
@lopopolo lopopolo added A-ruby-core Area: Ruby Core types. A-spec Area: ruby/spec infrastructure and completeness. labels Feb 19, 2023
b-n and others added 2 commits February 19, 2023 20:17
Co-authored-by: Ryan Lopopolo <rjl@hyperbo.la>
Co-authored-by: Ryan Lopopolo <rjl@hyperbo.la>
@b-n b-n merged commit b3012e5 into trunk Feb 19, 2023
@b-n b-n deleted the b-n/month-string-values branch February 19, 2023 19:35
lopopolo added a commit that referenced this pull request Feb 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ruby-core Area: Ruby Core types. A-spec Area: ruby/spec infrastructure and completeness.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants