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

Remove unnecessary lifetimes of CRC digest #129

Merged
merged 1 commit into from
May 13, 2024

Conversation

cdunster
Copy link
Contributor

The CRC digest lifetime is set to the same as the provided buffer lifetime which means that the CRC has to live as long as the buffer which I don't think is necessary as the finalize() method is called on the digest. This strictness of the lifetime meant that I couldn't create a Crc in the local scope i.e. a method and then just return the bytes (see the new unit test for an example of what I mean).

Clippy suggests this change and to remove a few other lifetimes too so I think this should be a low risk change 🙂

Thanks for the great crate!

The CRC digest lifetime was too strict for using it in a method and it can be elided.
Copy link

netlify bot commented Jan 31, 2024

Deploy Preview for cute-starship-2d9c9b canceled.

Name Link
🔨 Latest commit 3d404e9
🔍 Latest deploy log https://app.netlify.com/sites/cute-starship-2d9c9b/deploys/65ba5a492f5a530008c1ca2a

@jamesmunns
Copy link
Owner

CC @huntc - do you know if these lifetimes were chosen intentionally?

@jamesmunns
Copy link
Owner

Also, I need to figure out if removing a lifetime like this is a breaking API change. If yes, it might need to wait for #128, but I don't think it is?

@cdunster
Copy link
Contributor Author

Also, I need to figure out if removing a lifetime like this is a breaking API change. If yes, it might need to wait for #128, but I don't think it is?

I would assume that removing a lifetime like this is not breaking because '_ could still live as long as 'a but it no longer has to, right? Maybe going the other way would be
a breaking change as the public interface would become more strict.

That's just my thought though and I'm not 100% sure 😅

@huntc
Copy link
Contributor

huntc commented Jan 31, 2024

I've just pulled down the latest commit on main, updated my toolchain to the latest (rustc 1.75.0 (82e1608df 2023-12-21) ), and performed a cargo clippy.

All I see that clippy recommends is the following:

warning: use of `default` to create a unit struct
   --> src/ser/serializer.rs:377:50
    |
377 |                     .map_err(|_| core::fmt::Error::default())
    |                                                  ^^^^^^^^^^^ help: remove this call to `default`
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#default_constructed_unit_structs
    = note: `#[warn(clippy::default_constructed_unit_structs)]` on by default

warning: the following explicit lifetimes could be elided: 'a
   --> src/ser/mod.rs:269:18
    |
269 | pub fn to_extend<'a, T, W>(value: &'a T, writer: W) -> Result<W>
    |                  ^^                ^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes
    = note: `#[warn(clippy::needless_lifetimes)]` on by default
help: elide the lifetimes
    |
269 - pub fn to_extend<'a, T, W>(value: &'a T, writer: W) -> Result<W>
269 + pub fn to_extend<T, W>(value: &T, writer: W) -> Result<W>

The explicit lifetime for to_extend is not required. However, I don't see that in this PR.

Regarding the lifetimes on Digest, the _ lifetime should be fine. I don't think this would be a breaking a change as the lifetime there relates to the Crc. I'm not sure why I originally associated the lifetime with the buffer.

@huntc
Copy link
Contributor

huntc commented Mar 12, 2024

Should we progress this pull request?

@jamesmunns jamesmunns merged commit 66f3f67 into jamesmunns:main May 13, 2024
4 checks passed
@jamesmunns
Copy link
Owner

Thank you all!

@cdunster cdunster deleted the remove-unnecessary-lifetimes branch June 3, 2024 09:26
@cdunster cdunster restored the remove-unnecessary-lifetimes branch June 3, 2024 09:26
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.

3 participants