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

crc32: mark some unsafe functions properly, add some safety documentation #236

Merged
merged 1 commit into from
Oct 25, 2024

Conversation

inahga
Copy link
Contributor

@inahga inahga commented Oct 24, 2024

Some small nits I encountered in reviewing this code. Shoudn't be any actual functional changes.

  • Any function/method that invokes an intrinsic on aarch64 or x86_64 is unsafe, because it has the precondition of ensuring the runtime processor supports the instructions, otherwise the result is UB.

    Results in a bit of an annoying proliferation of unsafe in pclmuqdg::Accumulator. There may be some fiddling that can be done to reduce the unsafe footprint, but it doesn't seem like a big deal for internal code.

  • I set warn(unsafe_op_in_unsafe_fn), since otherwise unsafe operations can "hide" in unsafe blocks and thus not receive scrutiny.

  • Added a couple of SAFETY comments. I eschewed safety comments for things that struck me as obvious, e.g. taking pointer to a slice that's just fed into an intrinsic.

(@folkertdev let me know if this kind of PR helps. This is all just techniques I was doing to review anyway, so figured they would be useful upstream too).

Copy link
Collaborator

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

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

this is very helpful, thanks!

@folkertdev folkertdev merged commit 2676c84 into trifectatechfoundation:main Oct 25, 2024
18 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.

2 participants