Default selection of field/scalar inverse implementation #331

No description provided.

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

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

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 and USE_FIELD_INV_NUM and just use !defined(USE_NUM_NONE) directly in the inverse functions.

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.

I'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 Opinion?

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:

diff --git a/configure.ac b/configure.ac
index 786d8dc..0f046f7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -275,14 +275,6 @@ esac
 case $set_bignum in
 gmp)
   AC_DEFINE(HAVE_LIBGMP, 1, [Define this symbol if libgmp is installed])
-  AC_DEFINE(USE_NUM_GMP, 1, [Define this symbol to use the gmp implementation for num])
-  AC_DEFINE(USE_FIELD_INV_NUM, 1, [Define this symbol to use the num-based field inverse implementation])
-  AC_DEFINE(USE_SCALAR_INV_NUM, 1, [Define this symbol to use the num-based scalar inverse implementation])
-  ;;
-no)
-  AC_DEFINE(USE_NUM_NONE, 1, [Define this symbol to use no num implementation])
-  AC_DEFINE(USE_FIELD_INV_BUILTIN, 1, [Define this symbol to use the native field inverse implementation])
-  AC_DEFINE(USE_SCALAR_INV_BUILTIN, 1, [Define this symbol to use the native scalar inverse implementation])
   ;;
 *)
   AC_MSG_ERROR([invalid bignum implementation])
diff --git a/src/libsecp256k1-vars.h b/src/libsecp256k1-vars.h
new file mode 100644
index 0000000..646b81d
--- /dev/null
+++ b/src/libsecp256k1-vars.h
@@ -0,0 +1,18 @@
+#ifndef LIBSECP256K1_VARS_H
+#define LIBSECP256K1_VARS_H
+
+#if defined HAVE_CONFIG_H
+#include "libsecp256k1-config.h"
+#endif
+
+#if defined(HAVE_LIBGMP)
+#define USE_NUM_GMP 1
+#define USE_FIELD_INV_NUM 1
+#define USE_SCALAR_INV_NUM 1
+#else
+#define USE_NUM_NONE 1
+#define USE_FIELD_INV_BUILTIN 1
+#define USE_SCALAR_INV_BUILTIN 1
+#endif
+
+#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.

Closing this, but feel free to implement @theuni's suggestion.