Skip to content

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
@apoelstra apoelstra deleted the 2024-09--fix-docs branch September 12, 2024 19:29
chain-forgexcr45 added a commit to chain-forgexcr45/rust-secp256k1 that referenced this pull request Sep 28, 2025
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
william2332-limf added a commit to william2332-limf/rust-secp256k1 that referenced this pull request Oct 2, 2025
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
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