Add localtime_r shim - Githubissues

rust-lang / miri

An interpreter for Rust's mid-level intermediate representation

Apache License 2.0

4.18k stars 323 forks source link

Add localtime_r shim #3461

Closed tiif closed 2 months ago

tiif commented 3 months ago

Implement localtime_r shim as mentioned in #2057

Note:

tm_zone, tm_gmtoff might not be consistent with libc::localtime_r as custom implementation is provided through chrono. Due to the lack of daylight saving information in chrono, is_dst value will always be -1.

tiif commented 3 months ago

@RalfJung @oli-obk If memory leak in tm_zone string is a concern, would it be better if we just don't update the tm_zone field at all? The implementation provided is not accurate most of the time anyway since I didn't manage to use timezone abbreviation, I put something like +08 instead.

tiif commented 3 months ago

Thanks for the thorough review! I need a little bit more time to fully address them, and will mark this pr as ready after that.

tiif commented 3 months ago

Sorry for submitting so many comments one go, I didn't notice that these comments are under pending, and only visible to me :(

rustbot commented 3 months ago

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

099e245df3fe1ee8c680a866afa292c10ea2655a
ef7eb6eed1c72f34cc14cf54985429a74540f66d
942b6243e3f0c407aabe69dd6a07a7a22b3bccc3
f1f5c39e8ef94e70af57da649a9d07102471e5fc
e6a41a74dabe1af87dd09aa3dcce844e0664f0f1
8f7afb88371721465f62b7f850a2c264dad3ca23

RalfJung commented 3 months ago

@rustbot author

rustbot commented 3 months ago

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

62fc4a8e89cddd3b69d5f532abce3a64d3ebc958
71e04b9f711f6b40f09e759fb755b37a7996991f

tiif commented 3 months ago

@rustbot ready

rustbot commented 2 months ago

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits (since this message was last posted):

d2a0ab26d63bdec3c4ef6a748023cfed8b603ad0

RalfJung commented 2 months ago

All right, looks good. Thanks! Please rebase and squash.

RalfJung commented 2 months ago

@bors r+

bors commented 2 months ago

:pushpin: Commit 90b408c9d4fdf75dd50e0f2842604898c43351b4 has been approved by RalfJung

It is now in the queue for this repository.

bors commented 2 months ago

:hourglass: Testing commit 90b408c9d4fdf75dd50e0f2842604898c43351b4 with merge 887088106bbe062b2e664b260b61bbc244cd64b8...

tiif commented 2 months ago

Thanks @RalfJung and @oli-obk for mentoring this PR! I really learned a lot from the detailed suggestion and discovered a lot of new Rust syntax that I never used before. I will be away preparing for final exams until 1 May, and I will start working on socketpair by then.

bors commented 2 months ago

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing 887088106bbe062b2e664b260b61bbc244cd64b8 to master...

bors commented 2 months ago

:sunny: Test successful - checks-actions Approved by: RalfJung Pushing 887088106bbe062b2e664b260b61bbc244cd64b8 to master...