Skip to content

Conversation

@peter-formlogic
Copy link
Contributor

What is the purpose of the change

This is a follow-on from an early PR which eliminates the use of unsafe via static mut by using atomic values instead.

This adjusts the implementation of the max allocation bytes & serde human readable to use appropriate atomics for both.

This does have the added benefit of being able to adjust these values multiple time as the program runs, rather than setting them once.

Verifying this change

Tests have been adjusted slightly to use the set_serde_human_readable function

Documentation

I've updated the documentation to exclude the note about Once as this is no longer used and it is possible to update the allocation & human readable on the fly as your library runs.

@github-actions github-actions bot added the Rust label May 11, 2023
@peter-formlogic
Copy link
Contributor Author

Also, just to alleviate any concerns about differences in semantics, godbolt shows this compiles basically the same as the static mut variant:

https://godbolt.org/z/o3xzrb5q6

@martin-g
Copy link
Member

I am on the fence about dropping the usage of Once here.
I see the reason why the original author made use of it!
I also understand that one may need more flexibility but it may also cause problems, e.g. if two or more libraries used by an application decide to set different values for a setting. Currently by using Once the application can set the preferred value that would work for both used libraries.

@peter-formlogic
Copy link
Contributor Author

My read of it was that it was just because static muts are a pain to get right is why Once was used. We can probably combine Once with atomics if that's intended behaviour & still avoid having unsafe.

@martin-g
Copy link
Member

I am not the original author of this part of the code but I think Once is used to prevent third parties to overwrite the preferred value by the application.

I'll close this PR.
We can reconsider dropping the usage of Once when someone have a convincing reason for that.
Thank you for the contribution!

@martin-g martin-g closed this May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants