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

PROTOTYPE Split headers to make it possible to use only prealloc API #1166

Closed

Conversation

real-or-random
Copy link
Contributor

On top of #1126.

This prototypes one possible approach to solve #1095 by implementing the approach described in #1095 (comment):

[...] reorganize headers such that most functions are in some header like secp256k1_main.h, which is then included from secp256k1.h (with only malloc functions like secp256k1_context_create()) and secp256k1_preallocated.h (with their prealloc counterparts). Then the user could easily choose between the two interfaces by including either secp256k1.h or secp256k1_preallocated.h

Now that I've been working on this, I don't like this approach for various reasons:

  • The functions and docs are really cluttered in multiple files.
    • In particular, the context stuff: Creation, cloning, destruction is in secp256k1_(preallocated).h, whereas the static context, flags, randomization, and callbacks are in secp256k1_main.h. This is hard to grasp for new users and also add confusion for users who don't need the preallocated interface.
    • In general, there's no canonical "primary" header anymore. Where do you put general docs that belong to the library as a whole (e.g., the comment that explains the rules of thumb for the order of arguments in API calls)?
  • We need to remove the #include "secp256k1.h" from the module headers such as secp256k1_ecdh.h (because otherwise you couldn't include the module header if you want only the prealloc interface). This means that
    • this PR is not perfectly backwards-compatible: it will break compilation for users who include currently only secp256k1_ecdh.h and not secp256k1_ecdh.h.
    • the dependencies between the header files are not naturally expressed by the #includes
    • you need to include the header files in the right order, i.e., first secp256k1_(preallocated).h and then secp256k1_ecdh.h because the latter needs the declaration of secp256k1_context (and we couldn't even repeat that declaration somewhere to workaround this problem: Compiler tells us warning: redefinition of typedef 'secp256k1_context' is a C11 feature...)

So I think this PR would indeed make it possible to use only the prealloc interface, but it makes the life harder for everyone, in particular for normal users who have malloc and don't even care about prealloc. I think we should switch to an approach with a macro like PREALLOC_INTERFACE_ONLY that you need to set before including secp256k1.h (and when compiling the library itself) and that will remove all the functions that depend on malloc. But please tell me if you disagree.

@real-or-random
Copy link
Contributor Author

Closing to the reasons explained above. It turns out that this is not the best approach to solve #1095 ,

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