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

fix docs for new clippy lint #740

Merged
merged 2 commits into from
Sep 12, 2024

Conversation

apoelstra
Copy link
Member

There are a bunch of doccomments whose first lines are (much) too long. Most of these are also difficult to understand and/or out-of-date. Just rewrite them all.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 12, 2024

Concept ACK but it also fails CI. :)

@apoelstra
Copy link
Member Author

strace shows that xargo is failing trying to access some internal lockfile:

/store/home/apoelstra/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/Cargo.lock

Looks to me like xargo is just broken and I need to revert the update to the nightly compiler.

/// If you create one of these with `secp256k1_context_create` you must
/// destroy it with `secp256k1_context_destroy`. (Failure to destroy it is
/// a memory leak; destroying it using any other allocator is likely to be
/// undefined behavior.)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd just write "is undefined behavior". I don't think it's useful to try to define some crazy circumstances when it's not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

/// secret key passed to `ElligatorSwift::shared_secret`.
/// Represents which party we are in the ECDH.
///
/// Here `A` is the initiator and `B` is the responder.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we should've named the enum variants as such. :(

Copy link
Member Author

Choose a reason for hiding this comment

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

/// of being zero, overflowing the group order, or equalling any specific value.
///
/// Since version 0.29 this has been deprecated; users should instead implement
/// `Into<Message>` for types that satisfy these properties.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder when are we going to remove this entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was deprecated in April. Let's give it at least a year (unless it winds up blocking 1.0). I'd give it 2. It's self-contained and not hurting anything.

Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

Successfully ran local tests on cff0e2e.

@apoelstra
Copy link
Member Author

Yeah -- I believe this is fixed upstream by rust-lang/cargo#14370 so we just need to wait. Meanwhile I'll revert the nightly update.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 12, 2024

That issue says august 8th.

@apoelstra
Copy link
Member Author

Yes. How long is the delay between stuff getting merged into cargo and it showing up in a rustc nightly? I have no idea.

Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

Successfully ran local tests on 9e08c87.

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 12, 2024

How long is the delay between stuff getting merged into cargo and it showing up in a rustc nightly?

IIUC it's should be the following night - hence the name "nightly"

@apoelstra
Copy link
Member Author

I think that only applies to rustc itself. Everything else has a delay. In particular clippy has a delay, and ISTR that one of their developers told us it was because the clippy changes would ship alongside the cargo changes.

@apoelstra
Copy link
Member Author

Reminder to me to merge #737 after this gets in

@Kixunil
Copy link
Collaborator

Kixunil commented Sep 12, 2024

Do you want to address my comments?

There are a bunch of doccomments whose first lines are (much) too long.
Most of these are also difficult to understand and/or out-of-date. Just
rewrite them all.
@apoelstra
Copy link
Member Author

Sure. Removed "likely to be" from the UB comment. Should be good to go now.

Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

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

ACK 3810686

Copy link
Member Author

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

Successfully ran local tests on 3810686.

@apoelstra apoelstra merged commit 909fcd5 into rust-bitcoin:master Sep 12, 2024
30 checks passed
@apoelstra apoelstra deleted the 2024-09--fix-docs branch September 12, 2024 19:29
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