Default selection of field/scalar inverse implementation

[chfast]

No description provided.

[apoelstra]

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?

[chfast]

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?

[apoelstra]

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.

[chfast]

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]

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.

[sipa]

@theuni Opinion?

[theuni]

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.

[sipa]

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