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

Remove uses of unsafe. #142

Merged
merged 4 commits into from
Apr 4, 2020
Merged

Remove uses of unsafe. #142

merged 4 commits into from
Apr 4, 2020

Conversation

jswrenn
Copy link
Contributor

@jswrenn jswrenn commented Mar 22, 2020

Fixes #126 by completely removing all instances of unsafe. Instead, Unsigned values are flowed through the methods core::ops and type_operator traits. The appropriate methods have been added to the Private* traits, as needed.

This pull request takes an aggressive approach, but it may not be desirable to remove all instances of unsafe. This pull request:

  • makes a breaking? change to TArr
  • introduces a small number of internal type bounds, which could impact performance.

I'll spotlight where these changes occur with pull-request comments.

@jswrenn jswrenn force-pushed the look-ma-no-unsafe branch 2 times, most recently from b83370b to a2c1ea1 Compare March 22, 2020 22:40
@jswrenn jswrenn force-pushed the look-ma-no-unsafe branch from a2c1ea1 to 9ce20ce Compare March 22, 2020 22:46
Copy link
Contributor Author

@jswrenn jswrenn left a comment

Choose a reason for hiding this comment

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

I've highlighted important changes with individual comments. These include:

  • design decisions
  • possible performance footguns
  • changes potentially affecting stability

src/array.rs Show resolved Hide resolved
src/array.rs Show resolved Hide resolved
src/int.rs Outdated Show resolved Hide resolved
src/uint.rs Show resolved Hide resolved
src/uint.rs Outdated
Comment on lines 1225 to 1226
where
Bn: Copy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Bn: Copy bound is needed so we can produce an instance of the least significant bit from a reference to a UInt.

Copy link
Owner

Choose a reason for hiding this comment

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

I have no objection to this, but couldn't we alternatively make get_bit take things by value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could! We'd just need to add an extra Copy bound to the PartialDiv* implementations.

src/marker_traits.rs Show resolved Hide resolved
src/uint.rs Show resolved Hide resolved
src/uint.rs Show resolved Hide resolved
src/type_operators.rs Show resolved Hide resolved
@jswrenn
Copy link
Contributor Author

jswrenn commented Mar 27, 2020

  • introduces a small number of internal type bounds, which could impact performance.

Per #63 (comment), this may not actually pose any performance issue.

Copy link
Owner

@paholg paholg 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 great, thank you for doing this!

Would you mind just running cargo fmt? That should be enough to get the build passing.

src/array.rs Show resolved Hide resolved
src/type_operators.rs Show resolved Hide resolved
src/uint.rs Outdated
Comment on lines 1225 to 1226
where
Bn: Copy
Copy link
Owner

Choose a reason for hiding this comment

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

I have no objection to this, but couldn't we alternatively make get_bit take things by value?

src/uint.rs Outdated Show resolved Hide resolved
@paholg
Copy link
Owner

paholg commented Apr 4, 2020

Thanks again for doing this, I am very excited to get it merged. :)

@paholg paholg merged commit 8b49f74 into paholg:master Apr 4, 2020
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.

Warnings on nightly about mem::uninitialized.
2 participants