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

recursively add toHash @safe and nothrow #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

WebFreak001
Copy link

@WebFreak001 WebFreak001 commented Jan 26, 2023

I removed heapValue from the VObjectKey union because it wasn't used and made it too difficult to prove safety.

Fixes build failures and CI failures such as these: https://github.com/snazzy-d/sdc/actions/runs/3970459790/jobs/6806208714

I removed heapValue from the VObjectKey union because it wasn't used
and made it too difficult to prove safety
@deadalnix
Copy link
Contributor

I think this is a definite step back:

  • It makes the code much more noisy.
  • It makes the code much less flexible. Now modifying the various bit tricks in there will not only involved figuring out the tricks, but also how to convince the compiler things are actually @safe, nothrow, etc...
  • It doesn't look like there is a strong reason for toHash to be attributes souped in this way.
  • This behavior was removed from DMD, so bending the codebase over backward to accommodate it seems like the wrong tradeof.

If anything needs to be solved here, it is DMD's glacial release pace.

PS: I expect this to break on LDC, because mulhi would not be inferred to be @safe there.

@WebFreak001
Copy link
Author

It makes the code much more noisy.

This is up to the developer / project guidelines how you make them seen, such as through formatting or by saying you put them (@safe at least) at the top of the file. When consistently using them it's not much more than just function boilerplate, that could just as easily be hidden / made less noisy by your IDE. The benefits when the whole project properly uses @safe are also bigger imo and consistently using nothrow allows devs to better expect which functions to try-catch on when doing things like entrypoints from user input / callbacks.

It makes the code much less flexible. Now modifying the various bit tricks in there will not only involved figuring out the tricks, but also how to convince the compiler things are actually @safe, nothrow, etc...

I don't think that's true. Bit tricks are at least always nothrow and I think the rules when to use @trusted over @safe aren't gonna stop most simple bit tricks, and in other cases adding @trusted makes you check the whole logic properly one more time and encourages wrapping them in dedicated data structures, both of which are a good thing anyway.

It doesn't look like there is a strong reason for toHash to be attributes souped in this way.

That's true when the code isn't used elsewhere / exposed as public API, but otherwise such restrictions are very useful for other library authors that might want to do some optimization. (e.g. HashMap container authors don't need to wrap each index access with try-catch + don't need to lie about safety or make an unsafe library)

I don't think it's the compilers job to give out this warning though, it should rather be part of the linter. (which would also of course fit very nicely into the compilation step, but we would need the ability to configure warnings per-project, per-module, per-scope, per-line if we went this route)

I think safety attributes have value in every project, especially when you start developing functionality on top of your bit hacks structs. When at that point it's nice to have @safe to prevent you from accidentally adding unsafe code that could easily crash your application or worse. This PR adds a few safety attributes already that should hopefully motivate to put more @safe on other and new methods as well. What's nice about the attributes in D is that you don't need to put them on the method callers, so it's not going out of control / doesn't force you to use them everywhere, though it's better style.

@deadalnix deadalnix force-pushed the master branch 2 times, most recently from f94baa8 to 87a9baf Compare August 23, 2023 22:14
@deadalnix deadalnix force-pushed the master branch 3 times, most recently from cf07b66 to 39f39ba Compare January 3, 2024 14:18
@deadalnix deadalnix force-pushed the master branch 2 times, most recently from 4acd1ab to e854d3d Compare June 17, 2024 21:01
@deadalnix deadalnix force-pushed the master branch 3 times, most recently from 0896895 to 5a79292 Compare October 13, 2024 11:52
@deadalnix deadalnix force-pushed the master branch 3 times, most recently from 7ce40a5 to 4dbe602 Compare December 4, 2024 01:18
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