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

Address #90: Add must_use message to pub Results #95

Merged
merged 7 commits into from
Sep 6, 2019

Conversation

colelawrence
Copy link
Contributor

  • Remove redundant must_use from internal fns

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5554a09). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #95   +/-   ##
=========================================
  Coverage          ?   95.95%           
=========================================
  Files             ?       36           
  Lines             ?     5594           
  Branches          ?        0           
=========================================
  Hits              ?     5368           
  Misses            ?      226           
  Partials          ?        0
Impacted Files Coverage Δ
src/hazardous/aead/chacha20poly1305.rs 89.31% <ø> (ø)
src/hazardous/aead/xchacha20poly1305.rs 85.32% <ø> (ø)
src/hazardous/mac/hmac.rs 96.65% <ø> (ø)
src/hazardous/hash/blake2b.rs 94.53% <ø> (ø)
src/hazardous/stream/xchacha20.rs 82.83% <ø> (ø)
src/hazardous/hash/sha512.rs 98.2% <ø> (ø)
src/hazardous/mac/poly1305.rs 96.1% <ø> (ø)
src/hazardous/stream/chacha20.rs 89.32% <ø> (ø)
src/util/mod.rs 100% <ø> (ø)
src/hazardous/kdf/hkdf.rs 98.78% <ø> (ø)
... and 1 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 5554a09...7d299ac. Read the comment docs.

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@5554a09). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #95   +/-   ##
=========================================
  Coverage          ?   95.97%           
=========================================
  Files             ?       36           
  Lines             ?     5594           
  Branches          ?        0           
=========================================
  Hits              ?     5369           
  Misses            ?      225           
  Partials          ?        0
Impacted Files Coverage Δ
src/hazardous/aead/chacha20poly1305.rs 89.02% <ø> (ø)
src/hazardous/aead/xchacha20poly1305.rs 85.32% <ø> (ø)
src/hazardous/mac/hmac.rs 96.65% <ø> (ø)
src/hazardous/hash/blake2b.rs 94.53% <ø> (ø)
src/hazardous/stream/xchacha20.rs 83.58% <ø> (ø)
src/hazardous/hash/sha512.rs 98.2% <ø> (ø)
src/hazardous/mac/poly1305.rs 96.1% <ø> (ø)
src/hazardous/stream/chacha20.rs 89.32% <ø> (ø)
src/util/mod.rs 100% <ø> (ø)
src/hazardous/kdf/hkdf.rs 100% <ø> (ø)
... and 1 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 5554a09...6fe2772. Read the comment docs.

Copy link
Member

@brycx brycx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small adjustments needed, please see the comments.

src/hazardous/hash/sha512.rs Show resolved Hide resolved
src/hazardous/mac/hmac.rs Show resolved Hide resolved
src/hazardous/mac/poly1305.rs Show resolved Hide resolved
src/hazardous/stream/chacha20.rs Show resolved Hide resolved
@brycx
Copy link
Member

brycx commented Sep 6, 2019

@colelawrence Do you know if it's possible to specify the warning message as a &'static str and have it saved in the errors module? If it is, I think that would be a better approach than having to edit every single one, in case we want to change it later.

Edit: CI complains about the formatting, are you running cargo fmt?

@colelawrence
Copy link
Contributor Author

@colelawrence Do you know if it's possible to specify the warning message as a &'static str and have it saved in the errors module? If it is, I think that would be a better approach than having to edit every single one, in case we want to change it later.

I'll look into that and implement your suggestions

@colelawrence
Copy link
Contributor Author

I haven't been running cargo fmt. I didn't know it was available.

I ran cargo fmt and this happened is that normal? Sorry, if there's something silly I'm missing...

@brycx
Copy link
Member

brycx commented Sep 6, 2019

I ran cargo fmt and this happened is that normal? Sorry, if there's something silly I'm missing...

I think it's some mis-formatting in rustfmt. But since we check that in CI, this is how it's "supposed" to look.

@colelawrence
Copy link
Contributor Author

colelawrence commented Sep 6, 2019

I asked about the static string idea, and I heard that we won't be able to use a static str or const within the attribute macro syntax due to different compilation steps.

However, it would be possible to create a #[orion_must_use] proc-macro for generating the same #[must_use=message]

I would guess it could be relatively simple, but I haven't looked into it beyond that.

@brycx
Copy link
Member

brycx commented Sep 6, 2019

I asked about the static string idea, and I heard that we won't be able to use a static str or const within the attribute macro syntax due to different compilation steps.

Thanks for looking into this. I think we can just keep this as-is for now. There aren't that many messages after all and I feel like using proc-macros would add unneeded complexity to something that is fairly simple.

Copy link
Member

@brycx brycx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thank you @colelawrence for working on this!

@brycx brycx merged commit 2b5106c into orion-rs:master Sep 6, 2019
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