Default selection of field/scalar inverse implementation #331
pull chfast wants to merge 1 commits into bitcoin-core:master from chfast:config changing 2 files +20 −0-
chfast commented at 8:55 pm on October 1, 2015: none
-
Default selection of field/scalar inverse implementation. 9cc0ad046a
-
apoelstra commented at 3:02 pm on October 2, 2015: contributorRight now isn’t the same logic in
configure.ac
? I wouldn’t mind moving it from there, but I also don’t see a strong reason to .. what is the motivation for this patch? -
chfast commented at 3:09 pm on October 2, 2015: noneMight be. That’s actually my point. I’m not using autoconf for building the project so logic in configure.ac is not useful to me. I would like to move as much of that logic to C code as possible. What do you think?
-
apoelstra commented at 3:16 pm on October 2, 2015: contributor
I have no strong feelings either way. I think that once #290 gets in these variables should go away (since we’ll always have a nontrivial num.h implementation), but we don’t really have a timeline for that right now. In the meantime I’ll defer to whoever decided to add these variables in the first place, as opposed to say, using USE_NUM_GMP and friends directly.
Regardless of concept ACK/NAK, I think you should change this patch to (a) remove the now-redundant logic from configure.ac, and (b) eliminate
USE_FIELD_INV_BUILTIN
andUSE_FIELD_INV_NUM
and just use!defined(USE_NUM_NONE)
directly in the inverse functions. -
chfast commented at 3:19 pm on October 2, 2015: none
Thanks. I will wait for other opinions.
I can change as you suggested, but in such case we will loose
USE_NUM_GMP
+USE_*_INV_BUILTIN
configs. I don’t care, but it might be good for testing. -
gmaxwell commented at 11:45 pm on October 3, 2015: contributorI’m not fond of where this is done (in the middle of function implementations and repeated), but moreover– I don’t think it’s a great idea. There is a list of things the build system must set. Having one more or less doesn’t really change anything. We could document more clearly what must be set, and how– and I think that would accomplish a lot more towards both making things easy and making sure things are setup sensibly.
-
theuni commented at 4:58 pm on October 12, 2015: contributor
I’m really not a fan of that logic. I’ve had very bad experiences in the past with overriding autoconf variables. It leads to spaghetti very quickly.
If anything, I’d prefer to add a wrapper for libsecp256k1-config.h that takes advantage of autoconf vars when possible, and assumes a lack of support unless explicitly defined outside of autotools. Something like:
0diff --git a/configure.ac b/configure.ac 1index 786d8dc..0f046f7 100644 2--- a/configure.ac 3+++ b/configure.ac 4@@ -275,14 +275,6 @@ esac 5 case $set_bignum in 6 gmp) 7 AC_DEFINE(HAVE_LIBGMP, 1, [Define this symbol if libgmp is installed]) 8- AC_DEFINE(USE_NUM_GMP, 1, [Define this symbol to use the gmp implementation for num]) 9- AC_DEFINE(USE_FIELD_INV_NUM, 1, [Define this symbol to use the num-based field inverse implementation]) 10- AC_DEFINE(USE_SCALAR_INV_NUM, 1, [Define this symbol to use the num-based scalar inverse implementation]) 11- ;; 12-no) 13- AC_DEFINE(USE_NUM_NONE, 1, [Define this symbol to use no num implementation]) 14- AC_DEFINE(USE_FIELD_INV_BUILTIN, 1, [Define this symbol to use the native field inverse implementation]) 15- AC_DEFINE(USE_SCALAR_INV_BUILTIN, 1, [Define this symbol to use the native scalar inverse implementation]) 16 ;; 17 *) 18 AC_MSG_ERROR([invalid bignum implementation]) 19diff --git a/src/libsecp256k1-vars.h b/src/libsecp256k1-vars.h 20new file mode 100644 21index 0000000..646b81d 22--- /dev/null 23+++ b/src/libsecp256k1-vars.h 24@@ -0,0 +1,18 @@ 25+#ifndef LIBSECP256K1_VARS_H 26+#define LIBSECP256K1_VARS_H 27+ 28+#if defined HAVE_CONFIG_H 29+#include "libsecp256k1-config.h" 30+#endif 31+ 32+#if defined(HAVE_LIBGMP) 33+#define USE_NUM_GMP 1 34+#define USE_FIELD_INV_NUM 1 35+#define USE_SCALAR_INV_NUM 1 36+#else 37+#define USE_NUM_NONE 1 38+#define USE_FIELD_INV_BUILTIN 1 39+#define USE_SCALAR_INV_BUILTIN 1 40+#endif 41+ 42+#endif
Then, all current code would include “libsecp256k1-vars.h” instead of “libsecp256k1-config.h”. Non-autotools projects would
-DHAVE_LIBGMP
to indicate use of the lib. -
sipa closed this on Nov 1, 2015
This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 05:15 UTC
More mirrored repositories can be found on mirror.b10c.me