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

argon2: more const-ness #450

Merged
merged 6 commits into from
Aug 26, 2023
Merged

argon2: more const-ness #450

merged 6 commits into from
Aug 26, 2023

Conversation

C0D3-M4513R
Copy link
Contributor

@C0D3-M4513R C0D3-M4513R commented Aug 11, 2023

Redo of #438 after #440.
Rebased original PR, then applied fixes and then sqashed

argon2/src/params.rs Outdated Show resolved Hide resolved
argon2/src/params.rs Outdated Show resolved Hide resolved
@C0D3-M4513R
Copy link
Contributor Author

C0D3-M4513R commented Aug 16, 2023

With both above approaches out of the question I see no other alternative than to make a breaking change here and wait for a merge with the next breaking change.

Also I don't necessarily think that opening an issue to go alongside this is a good idea.
Why have two places to discuss things, rather than one?

@tarcieri
Copy link
Member

I think this PR could've benefitted from some up-front design discussion.

I'm not sure it necessarily makes sense to constify the builder. And doing so, at least the way you propose, means copying the entire thing every time you use any builder method. There are some tradeoffs there I think are worth discussing.

@C0D3-M4513R
Copy link
Contributor Author

C0D3-M4513R commented Aug 16, 2023

Well I don't want to duplicate any code from the Builder's build method in the Param's new method.
And with the state of const fn as they are right now, I do not see a way of doing so without either of the ways I proposed, which would be:

  • adding a new new method to the builder that works in const contexts
  • Making the Builder const compatible

Since I do not see us coming on a common factor I will just keep using my fork of password-hashes argon2 for now until something is implemented.

Also the change 6e9662a would not copy the Builder, it moves it which is something different.
Also in practice it should make no difference, since I am returning the same builder anyways.
So the variable the builder is assigned to does not actually change.

@tarcieri
Copy link
Member

Well I don't want to duplicate any code from the Builder's build method in the Param's new method.

Okay, so it seems like part of the problem for now is Params::new is using the builder internally in its current implementation.

But Params::new is much more ideal for constification since it already accepts all of the parameters explicitly.

Perhaps instead of trying to constify the Builder, you could eliminate the internal use of it within Params and make const fn new which could then in turn be used by the Builder, rather than the other way around.

@C0D3-M4513R C0D3-M4513R reopened this Aug 16, 2023
@C0D3-M4513R
Copy link
Contributor Author

Changed as requested.

@C0D3-M4513R C0D3-M4513R changed the title Redo of my other constness pr argon2: more const-ness Aug 18, 2023
@tarcieri tarcieri merged commit d25ea61 into RustCrypto:master Aug 26, 2023
13 checks passed
@tarcieri
Copy link
Member

Thank you!

@tarcieri tarcieri mentioned this pull request Sep 4, 2023
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