Skip to content

Conversation

@Urgau
Copy link
Member

@Urgau Urgau commented Jun 20, 2024

This PR improves the documentation of the public methods of StableHasher and adds some usage examples.

cc @weihanglo
r? @WaffleLapkin
Fixes #2

/// which returns a `(u64, u64)` pair for greater precision.
///
/// </div>
fn finish(&self) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

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

Does deprecated work on implementations? If so, you could add it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately no,

error: this `#[deprecated]` annotation has no effect
   --> src/stable_hasher.rs:118:5
    |
118 |     #[deprecated = "use StableHasher::finalize instead"]
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove the unnecessary deprecation attribute
    |
    = note: `#[deny(useless_deprecated)]` on by default

error: could not compile `rustc-stable-hash` (lib) due to 1 previous error

Copy link
Member

Choose a reason for hiding this comment

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

Afaik, the panic is from a time when there was no StableHashResult yet. It should be fine to just return one half of the hash or do something like Fingerprint::combine.

@Urgau
Copy link
Member Author

Urgau commented Jun 24, 2024

#4 was merged, I have rebased this PR over main, CI is green and I think #3 (comment) can be handled as a follow-up (rather than holding this PR). I think this is ready be merged.

@WaffleLapkin @michaelwoerister any objections?

@michaelwoerister
Copy link
Member

@Urgau, I left a couple of remarks but nothing blocking. I'm good with merging the PR, looks great!

@WaffleLapkin WaffleLapkin merged commit c6795f4 into rust-lang:main Jun 25, 2024
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.

Document public items

4 participants