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

API documentation improvements #289

Merged
merged 2 commits into from
Aug 27, 2015
Merged

API documentation improvements #289

merged 2 commits into from
Aug 27, 2015

Conversation

sipa
Copy link
Contributor

@sipa sipa commented Aug 20, 2015

No description provided.

@dcousens
Copy link
Contributor

Reviewed with w=0?.

utACK

* privkey: pointer to a private key in DER format (cannot be NULL).
* privkeylen: length of the DER private key pointed to be privkey.
* Out: seckey: pointer to a 32-byte array for storing the private key.
* (cannot be NULL).
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this functions in/out parameters are not in the same order as the documentation? Not that big an issue, just looked like a convention.

edit: upon further review, it seems there is no such convention being used here. Should there be though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd to have a convention of having all functions AND all documentation to have order: ctx/out/inout/in or ctx/in/inout/out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm personally in favour of ctx/in/inout/out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a convention within C APIs to use out/in order, to mimic assignment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that makes sense, but, I wouldn't be able to change the habit of reading things in terms of INPUT -> OUTPUT.

It'd be similar to seeing an anonymous function callback being used with the callback passed before arguments to the function.

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer out/in order, it is extremely common in C API's.

@sipa
Copy link
Contributor Author

sipa commented Aug 20, 2015

Addressed nits.


void bench_context_sign(void* arg) {
(void)arg;
int i;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm getting a "mixed declarations and code" warning for having int i after the (void)arg. Ditto in the above function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@apoelstra
Copy link
Contributor

ACK

@gmaxwell
Copy link
Contributor

ACK.

@sipa sipa merged commit f66907f into bitcoin-core:master Aug 27, 2015
sipa added a commit that referenced this pull request Aug 27, 2015
f66907f Improve/reformat API documentation secp256k1.h (Pieter Wuille)
2f77487 Add context building benchmarks (Pieter Wuille)
* guaranteed to be portable between different platforms or versions. It is
* however guaranteed to be 65 bytes in size, and can be safely copied/moved.
* If you need to convert to a format suitable for storage or transmission, use
* the secp256k1_ecdsa_signature_serialize_* and
Copy link

Choose a reason for hiding this comment

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

After-the-fact nit: "secp256k1_ecdsa_signature_serialize_*" appears twice. I'm guessing this is a mistake.

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.

5 participants