API documentation improvements #289

pull sipa wants to merge 2 commits into bitcoin-core:master from sipa:apidoc changing 2 files +280 −145

sipa commented at 1:16 AM on August 20, 2015: contributor

No description provided.
dcousens commented at 1:21 AM on August 20, 2015: contributor

Reviewed with w=0?.

utACK
in include/secp256k1.h:None in 6524fc33b7 outdated
```
 466 | + * Returns: 1 if a private key was extracted.
 467 | + * In:  ctx:        pointer to a context object (cannot be NULL).
 468 | + *      privkey:    pointer to a private key in DER format (cannot be NULL).
 469 | + *      privkeylen: length of the DER private key pointed to be privkey.
 470 | + * Out: seckey:     pointer to a 32-byte array for storing the private key.
 471 | + *                  (cannot be NULL).
```
dcousens commented at 1:24 AM on August 20, 2015:

~~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?

sipa commented at 5:45 PM on August 20, 2015:

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.

dcousens commented at 10:30 PM on August 20, 2015:

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

sipa commented at 11:03 PM on August 20, 2015:

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

dcousens commented at 12:33 AM on August 21, 2015:

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.

apoelstra commented at 2:37 PM on August 21, 2015:

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

in include/secp256k1.h:None in 6524fc33b7 outdated

 396 | -  const unsigned char *msg32,
 397 | -  const secp256k1_ecdsa_signature_t *sig,
 398 | -  secp256k1_pubkey_t *pubkey
 399 | +    const secp256k1_context_t* ctx,
 400 | +    const unsigned char *msg32,
 401 | +    const secp256k1_ecdsa_signature_t *sig,

dcousens commented at 1:24 AM on August 20, 2015:

Documentation calls this sig64, its sig here.

dcousens commented at 10:31 PM on August 20, 2015:

ping @sipa, missed nit or ?

sipa commented at 11:03 PM on August 20, 2015:

Fixed.

in src/bench_internal.c:None in 6524fc33b7 outdated
```
 295 | +    (void)arg;
 296 | +    int i;
 297 | +    for (i = 0; i < 200; i++) {
 298 | +        secp256k1_context_destroy(secp256k1_context_create(SECP256K1_CONTEXT_SIGN));
 299 | +    }
 300 | +}
```
dcousens commented at 1:26 AM on August 20, 2015:

For posterity, it should probably be mentioned this PR also adds 2 new benchmarks.

sipa commented at 9:03 PM on August 20, 2015:

Done, split into a separate commit.

in include/secp256k1.h:None in 6524fc33b7 outdated

  11 | + *  that are expensive to construct, and also to maintain the randomization data
  12 | + *  for blinding.
  13 | + *
  14 | + *  Do not create a new context object for each operation, as construction is
  15 | + *  far slower than all other API calls (~100 times slower than an ECDSA
  16 | + *  verification), and then use the context for all other operations.

apoelstra commented at 5:05 AM on August 20, 2015:

I think part of this sentence is missing; you say "do not do this .. and then" rather than "do not do this .. instead do this, and then".

in include/secp256k1.h:None in 6524fc33b7 outdated

  15 | + *  far slower than all other API calls (~100 times slower than an ECDSA
  16 | + *  verification), and then use the context for all other operations.
  17 | + *
  18 | + *  A constructed context can safely be used from multiple threads
  19 | + *  simultaneously, but some API calls need exclusive access, in particular
  20 | + *  secp256k1_context_destroy and secp256k1_context_randomize.

apoelstra commented at 5:18 AM on August 20, 2015:

Do we want to drop the comment that const makes the difference between requiring exclusive access and not?

sipa force-pushed on Aug 20, 2015
sipa commented at 7:35 PM on August 20, 2015: contributor

Addressed nits.
sipa force-pushed on Aug 20, 2015
Add context building benchmarks 2f77487012
Improve/reformat API documentation secp256k1.h f66907f220
in src/bench_internal.c:None in 6bde1dda2a outdated
```
 291 | +    }
 292 | +}
 293 | +
 294 | +void bench_context_sign(void* arg) {
 295 | +    (void)arg;
 296 | +    int i;
```
apoelstra commented at 3:06 PM on August 21, 2015:

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

sipa commented at 11:14 PM on August 26, 2015:

Fixed.
sipa force-pushed on Aug 26, 2015
sipa cross-referenced this on Aug 27, 2015 from issue Move ECDSA pubkey recovery key to a module by sipa
apoelstra commented at 6:52 PM on August 27, 2015: contributor

ACK
gmaxwell commented at 8:00 PM on August 27, 2015: contributor

ACK.
sipa merged this on Aug 27, 2015
sipa closed this on Aug 27, 2015
sipa referenced this in commit a7b046e554 on Aug 27, 2015

in include/secp256k1.h:None in f66907f220

  45 | + *
  46 | + *  The exact representation of data inside is implementation defined and not
  47 | + *  guaranteed to be portable between different platforms or versions. It is
  48 | + *  however guaranteed to be 65 bytes in size, and can be safely copied/moved.
  49 | + *  If you need to convert to a format suitable for storage or transmission, use
  50 | + *  the secp256k1_ecdsa_signature_serialize_* and

droark commented at 8:11 PM on August 29, 2015:

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

Contributors

Linked (view graph)

#291 Move ECDSA pubkey recovery key to a module