Define SECP256K1_BUILD in secp256k1.c directly. #928
Conversation
|
Does this risk accidentally including secp256k1.h somewhere before secp256k1.c, thus getting the wrong interface definitions? If so, perhaps it's a possibility to have .h define a |
|
Indeed and the PR fixed a couple tests that were doing exactly that. I didn't consider it much of a concern because the non-null warnings catch it in development... but you're absolutely right that this is easy to avoid and so I added your suggestion. |
This avoids building without it and makes it safer to use a custom building environment. Test harnesses need to #include secp256k1.c first now.
|
utACK ae9e648 |
| @@ -4,6 +4,8 @@ | |||
| * file COPYING or https://www.opensource.org/licenses/mit-license.php.* | |||
| ***********************************************************************/ | |||
|
|
|||
| #define SECP256K1_BUILD | |||
There was a problem hiding this comment.
Shouldn't this be wrapped in #ifndef SECP256k1_BUILD?
There was a problem hiding this comment.
I don't see why-- if it was you wouldn't have spotted that your rust build was incorrectly setting it now that it shouldn't be set that way anymore.
There was a problem hiding this comment.
I just thought it was common practice to wrap defines in ifndef in the case that the define acts as a flag and doesn't carry any additional value.
(I don't think there was any harm in the fact that we kept passing -DSECP256K1_BUILD to the compiler, as we don't really use "headers" in rust)
This avoids building without it and makes it safer to use a custom
building environment. Test harnesses need to #include secp256k1.c
first now.
Fixes #927