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

Added Makefile and using new_unchecked instead of new on NonZero types #216

Closed
wants to merge 2 commits into from
Closed

Conversation

unixpariah
Copy link

  1. Added a Makefile: This addition will help automate build processes, making it easier to compile and test the project.

  2. Updated NonZero types: The 'new' method has been replaced with 'new_unchecked' to bypass unnecessary checks. This method is unsafe as it does not verify if the value is zero, but in this context, none of the values should be zero.

@LGFae
Copy link
Owner

LGFae commented Feb 21, 2024

Sorry, but I don't like either of these changes.

The Makefile addition makes no sense; you've simply wrapped the more well know cargo commands behind an extra layer of abstraction. Furthermore, you've hard-coded the installation paths, with su. This is not a good approach for package maintainers. There may be distros who want to copy these files elsewhere, and probably without using su directly like this.

I only use unsafe for either truly performance sensitive code (like the compression/decompression functions, because they are the main drive of high cpu usage during animation), or because I simply can't do it without unsafe (like in some instances when using the rkyv library. Using unsafe in these NonZero types in these instances isn't useful, because:

  • the compiler will most likely be able to prove most of these instances are always correct, and optimize the extra checks away
  • if ever one day these things cease to be true, the code will explode in our face, like we want. With unsafe we would suddenly have a really annoying to track bug

You see, right now, the necessary invariants trivially hold (the values aren't zero). But, upon rewriting them with unsafe, they would have to trivially hold forever. Even if we go through large refactors, or if the upstream libraries change, or if some unforeseen circumstance arise, etc. For this reason, it is not a good idea to use unsafe functions even if right now you can prove they are correct; because from now on, you would have to keep proving that every time the code changes.

@LGFae LGFae closed this Feb 21, 2024
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.

None yet

2 participants