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

Removed unsafe #20

Closed
wants to merge 1 commit into from
Closed

Conversation

Enn3Developer
Copy link
Member

This pr resolves #15.
A lot of things needed to change but for the better.
First of all, I needed to add the lazy_static dependency, I used it to instantiate and hold all the data.
Then, I implemented a struct containing all the data and getters for it, that struct has even a convenient function for adding things to the output code.
I wrapped that struct inside a RwLock, if in the future the need arises (the other option was Mutex).
I created lib.rs that contains mods and macros and removed the arg macro as it is useless at this point (there's no more need for unsafe)
I added a configuration file for clippy because it was angry that EnvData::set_data had 10 arguments, 3 more of the max he tolerated.

@Maiori44
Copy link
Member

how does this impact performance? Does it slow it down or speed it up?

@Enn3Developer
Copy link
Member Author

It seems to be the same, but there should be an overhead when getting the lock

@Maiori44
Copy link
Member

and when does that happen?

@Enn3Developer
Copy link
Member Author

when calling ENV_DATA.read() or ENV_DATA.write()

@Maiori44
Copy link
Member

okay, I guess it's fine, I have 2 more questions:

  • wouldn't it be better to call panic!() once instead of .expect() every time?
  • shouldn't this pr be merged in the next branch instead?

@Enn3Developer
Copy link
Member Author

.expect() calls panic!() under the hood
about the branch, I don't know the differences between the two

@Maiori44
Copy link
Member

.expect() calls panic!() under the hood
I know, but if we call panic directly in the functions themselves I think we could save some memory in the exe?
about the branch, I don't know the differences between the two
the next branch is to be used for updates, the main branch should be updated when a new update is out (merging it with the next branch) or for small patches since the next branch is not necessary for those

@Enn3Developer
Copy link
Member Author

I know, but if we call panic directly in the functions themselves I think we could save some memory in the exe?

pub const fn expect(self, msg: &str) -> T {
    match self {
        Some(val) => val,
        None => expect_failed(msg),
    }
}

Either we do this in the code or call it through .expect() is the same (expect_failed directly calls panic)

the next branch is to be used for updates
Should I remake the pr for that branch?

@Maiori44
Copy link
Member

I guess we can keep .expect() then

Should I remake the pr for that branch?

yeah

@Enn3Developer
Copy link
Member Author

Closing to make the pr in the other branch

@Enn3Developer Enn3Developer mentioned this pull request Jun 17, 2022
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.

Unsafe code
2 participants