-
Notifications
You must be signed in to change notification settings - Fork 271
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
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.
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. |
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. |
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
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.
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.