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

More explicit struct initialization #87

Merged
merged 7 commits into from
Sep 14, 2019
Merged

More explicit struct initialization #87

merged 7 commits into from
Sep 14, 2019

Conversation

brycx
Copy link
Member

@brycx brycx commented Aug 31, 2019

Currently, for a given streaming context Ctx in module foo we have:

let _: Ctx = foo::init();

This PR changes this to be more explicit:

let _: Ctx = foo::Ctx::init();

@brycx brycx added improvement General improvements to code breaking change A breaking change labels Aug 31, 2019
@brycx brycx changed the title More explicit struct initialization. More explicit struct initialization Aug 31, 2019
@brycx
Copy link
Member Author

brycx commented Aug 31, 2019

What do you think @vlmutolo?

@codecov
Copy link

codecov bot commented Aug 31, 2019

Codecov Report

Merging #87 into master will increase coverage by 0.28%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage    96.2%   96.49%   +0.28%     
==========================================
  Files          36       36              
  Lines        5585     6297     +712     
==========================================
+ Hits         5373     6076     +703     
- Misses        212      221       +9
Impacted Files Coverage Δ
src/hazardous/kdf/hkdf.rs 100% <100%> (ø) ⬆️
src/hazardous/aead/chacha20poly1305.rs 92.08% <100%> (+2.77%) ⬆️
tests/hash/sha512_nist_cavp.rs 100% <100%> (ø) ⬆️
tests/mac/mod.rs 97.05% <100%> (ø) ⬆️
tests/hash/mod.rs 100% <100%> (ø) ⬆️
src/hazardous/kdf/pbkdf2.rs 98.86% <100%> (ø) ⬆️
src/hazardous/hash/blake2b.rs 93.08% <89.33%> (-1.46%) ⬇️
src/hazardous/mac/poly1305.rs 95.58% <95.45%> (-0.52%) ⬇️
src/hazardous/hash/sha512.rs 97.85% <96.66%> (-0.36%) ⬇️
src/hazardous/mac/hmac.rs 96.19% <96.87%> (-0.47%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73a21ea...37cbe91. Read the comment docs.

@vlmutolo
Copy link
Contributor

vlmutolo commented Sep 1, 2019

I think that the explicit version is much better. It definitely helps to immediately what the type of the newly-created object is.

The only question I have is whether it would be better to rename init to new, as is the Rust convention. But that would only be if the structs aren't ever really created in any other way. Meaning, is init the standard way to create these objects? I think it is, and therefor should be called new. But maybe I'm missing something.

@brycx
Copy link
Member Author

brycx commented Sep 1, 2019

The only question I have is whether it would be better to rename init to new, as is the Rust convention.

I agree this seems to be the way in Rust, though from what I have seen, using init() is more common throughout other crypto libraries which are not in Rust. Maybe the Rust API guidelines have something on this?

Edit: I couldn't find any information in the guidelines.

But that would only be if the structs aren't ever really created in any other way. Meaning, is init the standard way to create these objects? I think it is, and therefor should be called new. But maybe I'm missing something.

This is great, we'll rename init() to new().

@vlmutolo
Copy link
Contributor

vlmutolo commented Sep 4, 2019

Hey, quick turnaround! Nice. I think that this will be more intuitive for new users of the library. Less cognitive load wondering if init() is something special.

@brycx
Copy link
Member Author

brycx commented Sep 4, 2019

Unfortunately it seems to be a little more trouble than this. I didn't realize it, but clippy warns against new() constructors that don't return a Self or use &self. So Hasher in blake2b won't be able to have a new() method, but keep the old init(). We'll also need to add impl Default to Sha512. Adding Default is fine, but having Hasher being inconsistent with the rest seems a bit weird no?

Here's the CI output: https://travis-ci.org/brycx/orion/jobs/580625988

@brycx
Copy link
Member Author

brycx commented Sep 14, 2019

Taking into account that the Hasher in fact initializes a new Blake2b struct, I think it might not be that unclear after all. I'll accommodate the clippy suggestions, and keep the struct initialization to new().

@brycx brycx merged commit 8b5de76 into master Sep 14, 2019
@brycx brycx deleted the struct-init branch September 14, 2019 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change A breaking change improvement General improvements to code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants