API documentation improvements

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

dcousens commented at 1:21 am on August 20, 2015: contributor

Reviewed with w=0?.

utACK

in include/secp256k1.h: 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 0: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: 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: 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: 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: 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: 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

API documentation improvements #289