Skip to content

Add safety comments#43

Merged
MabezDev merged 1 commit into
rust-embedded-community:mainfrom
Manishearth:safety-comments
Apr 30, 2026
Merged

Add safety comments#43
MabezDev merged 1 commit into
rust-embedded-community:mainfrom
Manishearth:safety-comments

Conversation

@Manishearth

@Manishearth Manishearth commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Based on #42

This contains #42 but also adds safety comments everywhere.

cc @newAM

@MabezDev

Copy link
Copy Markdown
Member

I believe the original PR covers this.

@MabezDev MabezDev closed this Apr 28, 2026
@Manishearth

Copy link
Copy Markdown
Contributor Author

@MabezDev How so? This PR is adding comprehensive safety documentation. The other PR adds no documentation at all.

This repository has gotten unsafe code wrong not once, but twice, in the same bit of code. It currently has no safety comments, just comments explaining why unsafe was used. I did a safety review and found the latest bug.

Having safety comments helps people have confidence in your unsafe code, and helps future reviewers quickly audit the code.

It is harder to recommend the use of unsafe-using crates that don't have safety comments, especially when they have a track record of safety bugs that misunderstand basic things about UB in Rust. The MaybeUninit discrepancy is a common mistake to make; and whenever I see it usually it means that the code author thinks UB in Rust works like C++, and I've often found it paired with other instances of UB.

@MabezDev

Copy link
Copy Markdown
Member

I misunderstood this comment: #42 (review). Sorry about that, I'm just trying to keep things moving here with this crate.

@MabezDev MabezDev reopened this Apr 28, 2026

@newAM newAM left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me, thanks for adding these!

Sorry I didn't update my PR, didn't have the time.

@MabezDev MabezDev merged commit 8cdaca5 into rust-embedded-community:main Apr 30, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants