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

Refactor MacOS implementation a lot #67

Merged
merged 7 commits into from
Oct 8, 2022
Merged

Conversation

Kijewski
Copy link
Collaborator

Cf. #64, #65.

@astraw astraw added the Unsafe-Proposed `unsafe` code is added or changed label Sep 29, 2022
@astraw astraw mentioned this pull request Sep 29, 2022
@astraw
Copy link
Member

astraw commented Sep 29, 2022

@Kijewski I thought it was easier to read and safer if I moved each newtype to its own module. Along the way I did a small bit of refactoring. I have some concern this will not meet our MSRV requirements so may need to be updated once the CI tests are done. But it does actually run correctly on my Mac :)

Copy link
Collaborator

@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.

This LGTM. I left one proposed tweak to break apart a gnarly conditional.

src/tz_macos.rs Outdated Show resolved Hide resolved
@astraw
Copy link
Member

astraw commented Sep 29, 2022

This looks good to me. Thanks @Kijewski for the main work here. And the CI tests do all pass.

It seems we have a consensus amongst committers/reviewers that this is ready to merge. However, as it carries the Unsafe-Proposed label, let's let this sit for a week (so, until 6 Oct) to attract review as discussed in #64. Also, if someone is auditing this crate (e.g. rust-secure-code/safety-dance#79 ), I ask them to please check this implementation as an alternative to the existing macOS implementation.

@lopopolo
Copy link
Collaborator

Thanks all! This looks like a big improvement.

@Kijewski
Copy link
Collaborator Author

Kijewski commented Sep 30, 2022

I thought it was easier to read and safer if I moved each newtype to its own module.

I like that very much, @astraw. Now the types are truly self contained.

@astraw
Copy link
Member

astraw commented Sep 30, 2022

OK, as we changed the PR a bit with the new MSRV, keeping it open a week will now be 7 October.

@Kijewski Kijewski added the Tier-1 Rust Tier-1 platform label Oct 1, 2022
Copy link

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

I think this code doesn't have UB, but I do have a couple of suggestions to the safe code outside of system_time_zone and string_ref can't cause UB even when changing it.

(coming here from rust-secure-code/safety-dance#79)

src/tz_macos.rs Show resolved Hide resolved
src/tz_macos.rs Outdated Show resolved Hide resolved
src/tz_macos.rs Show resolved Hide resolved
@Kijewski
Copy link
Collaborator Author

Kijewski commented Oct 6, 2022

Sorry, I caught Corona and don't think I'll be able to do any coding, commenting or reviewing until early next week. :/

@astraw
Copy link
Member

astraw commented Oct 6, 2022

The CI failures on the last commits here (archive member 'rust.metadata.bin' with length 4847795 is not mach-o or llvm bitcode file) seem unrelated to the changes made. They arise on macOS with rustc 1.38 but not current stable or nightly. For this reason I propose we ignore them, but I'm really not sure what is going on.

@astraw
Copy link
Member

astraw commented Oct 8, 2022

The test failures are now documented in #72, which is unrelated.

Thanks everyone for the feedback and especially @Kijewski for the work on this. I hope your recovery is well underway.

@astraw astraw merged commit 5639513 into strawlab:main Oct 8, 2022
@Kijewski Kijewski deleted the pr-macos-safe2 branch October 15, 2022 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-1 Rust Tier-1 platform Unsafe-Proposed `unsafe` code is added or changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants