-
Notifications
You must be signed in to change notification settings - Fork 306
fix docs for new clippy lint #740
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
Conversation
|
Concept ACK but it also fails CI. :) |
|
strace shows that xargo is failing trying to access some internal lockfile: Looks to me like xargo is just broken and I need to revert the update to the nightly compiler. |
secp256k1-sys/src/lib.rs
Outdated
| /// 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.) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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. :(
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
apoelstra
left a comment
There was a problem hiding this 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.
|
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. |
|
That issue says august 8th. |
|
Yes. How long is the delay between stuff getting merged into cargo and it showing up in a rustc nightly? I have no idea. |
apoelstra
left a comment
There was a problem hiding this 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.
IIUC it's should be the following night - hence the name "nightly" |
|
I think that only applies to |
|
Reminder to me to merge #737 after this gets in |
|
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.
This reverts commit 78d93b7.
9e08c87 to
3810686
Compare
|
Sure. Removed "likely to be" from the UB comment. Should be good to go now. |
Kixunil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3810686
apoelstra
left a comment
There was a problem hiding this 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.
38106866c1c23ea2c76879cae2332ea80064711a Revert "Automated update to Github CI to rustc nightly-2024-09-10" (Andrew Poelstra)
d3d9a050a7c87807f6ccc978e8bf8bb2a8764f0b fix docs for new clippy lint. (Andrew Poelstra)
Pull request description:
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.
ACKs for top commit:
Kixunil:
ACK 38106866c1c23ea2c76879cae2332ea80064711a
Tree-SHA512: 291bd2c30c8d46c54d99eba17b6cc5f018912b906f4395fa753218551c1ba50724bdd55699f12bf9de254debf9612541c47e1fcd9c2eb04784f71c21e94b5ea5
38106866c1c23ea2c76879cae2332ea80064711a Revert "Automated update to Github CI to rustc nightly-2024-09-10" (Andrew Poelstra)
d3d9a050a7c87807f6ccc978e8bf8bb2a8764f0b fix docs for new clippy lint. (Andrew Poelstra)
Pull request description:
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.
ACKs for top commit:
Kixunil:
ACK 38106866c1c23ea2c76879cae2332ea80064711a
Tree-SHA512: 291bd2c30c8d46c54d99eba17b6cc5f018912b906f4395fa753218551c1ba50724bdd55699f12bf9de254debf9612541c47e1fcd9c2eb04784f71c21e94b5ea5
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.