[PATCH 1/2] Add conversions to and from struct timespec to module interface.

classic Classic list List threaded Threaded
56 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Add conversions to and from struct timespec to module interface.

Philipp Stephani
Time values are a fundamental data type, and such conversions are hard
to implement within modules because of the various forms of time
values in Emacs Lisp.  Adding dedicated conversion functions can
significantly simplify module code dealing with times.

This approach uses nanosecond precision.  While Emacs in theory has
support for higher-precision time values, in practice most languages
and standards, such as POSIX, C, Java, and Go, have settled on
nanosecond-precision integers to represent time.

* src/emacs-module.h.in: Add header for struct timespec.

* src/module-env-27.h: Add module functions for time conversion.

* src/emacs-module.c (module_extract_time, module_make_time): New
functions.
(initialize_environment): Use them.

* test/data/emacs-module/mod-test.c (Fmod_test_add_nanosecond): New
test function.
(emacs_module_init): Define it.

* test/src/emacs-module-tests.el (mod-test-add-nanosecond/valid)
(mod-test-add-nanosecond/invalid): New unit tests.

* doc/lispref/internals.texi (Module Values): Document time
conversion functions.
---
 doc/lispref/internals.texi        | 22 ++++++++++++++++++++++
 etc/NEWS                          |  3 +++
 src/emacs-module.c                | 20 ++++++++++++++++++++
 src/emacs-module.h.in             |  1 +
 src/module-env-27.h               |  6 ++++++
 test/data/emacs-module/mod-test.c | 13 +++++++++++++
 test/src/emacs-module-tests.el    | 25 +++++++++++++++++++++++++
 7 files changed, 90 insertions(+)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 25892d4b57..c2969d2cd1 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1387,6 +1387,15 @@ Module Values
 @var{arg}, as a C @code{double} value.
 @end deftypefn
 
+@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
+This function interprets @var{time} as an Emacs time value and returns
+the corresponding @code{struct timespec}.  @xref{Time of Day}.
+@var{time} may not be @code{nil}.  This function raises an error if
+@var{time} is out of range for @code{struct timespec}.  If @var{time}
+has higher precision than nanoseconds, then this function truncates it
+to nanosecond precision.
+@end deftypefn
+
 @deftypefn Function bool copy_string_contents (emacs_env *@var{env}, emacs_value @var{arg}, char *@var{buf}, ptrdiff_t *@var{len})
 This function stores the UTF-8 encoded text of a Lisp string specified
 by @var{arg} in the array of @code{char} pointed by @var{buf}, which
@@ -1452,6 +1461,19 @@ Module Values
 corresponding Emacs floating-point value.
 @end deftypefn
 
+@deftypefn Function emacs_value make_time (emacs_env *@var{env}, struct timespec @var{time})
+This function takes a @code{struct timespec} argument @var{time} and
+returns the corresponding Emacs timestamp.  @xref{Time of Day} for the
+possible return value formats.  It is not specified in which timestamp
+format the time is returned, but it is always a valid Emacs timestamp.
+The return value is exactly the same timestamp as @var{time}: all
+input values are representable, and there is never a loss of
+precision.  @code{time.tv_sec} and @code{time.tv_nsec} can be
+arbitrary values.  In particular, there's no requirement that @var{time}
+be normalized.  This means that @code{time.tv_nsec} doesn't have to be
+in the range [0, 999999999].
+@end deftypefn
+
 @deftypefn Function emacs_value make_string (emacs_env *@var{env}, const char *@var{str}, ptrdiff_t @var{strlen})
 This function creates an Emacs string from C text string pointed by
 @var{str} whose length in bytes, not including the terminating null
diff --git a/etc/NEWS b/etc/NEWS
index b13ab47768..2534262b62 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1910,6 +1910,9 @@ returns a regexp that never matches anything, which is an identity for
 this operation.  Previously, the empty string was returned in this
 case.
 
+** New module environment functions 'make_time' and 'extract_time' to
+convert between timespec structures and Emacs time values.
+
 
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 20dcff2b67..fc5a912d85 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -74,6 +74,7 @@ To add a new module function, proceed as follows:
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <time.h>
 
 #include "lisp.h"
 #include "dynlib.h"
@@ -731,6 +732,22 @@ module_process_input (emacs_env *env)
   return emacs_process_input_continue;
 }
 
+static struct timespec
+module_extract_time (emacs_env *env, emacs_value value)
+{
+  MODULE_FUNCTION_BEGIN ((struct timespec) {0});
+  Lisp_Object time = value_to_lisp (value);
+  CHECK_TYPE (!NILP (time), Qtimep, time);
+  return lisp_time_argument (time);
+}
+
+static emacs_value
+module_make_time (emacs_env *env, struct timespec time)
+{
+  MODULE_FUNCTION_BEGIN (NULL);
+  return lisp_to_value (env, make_lisp_time (time));
+}
+
 
 /* Subroutines.  */
 
@@ -1134,6 +1151,8 @@ initialize_environment (emacs_env *env, struct emacs_env_private *priv)
   env->vec_size = module_vec_size;
   env->should_quit = module_should_quit;
   env->process_input = module_process_input;
+  env->extract_time = module_extract_time;
+  env->make_time = module_make_time;
   Vmodule_environments = Fcons (make_mint_ptr (env), Vmodule_environments);
   return env;
 }
@@ -1296,6 +1315,7 @@ syms_of_module (void)
         build_pure_c_string ("Invalid function arity"));
 
   DEFSYM (Qmodule_function_p, "module-function-p");
+  DEFSYM (Qtimep, "timep");
 
   defsubr (&Smodule_load);
 }
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index 009d1583fe..bfbe226dd9 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -22,6 +22,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #include <stdint.h>
 #include <stddef.h>
+#include <time.h>
 
 #ifndef __cplusplus
 #include <stdbool.h>
diff --git a/src/module-env-27.h b/src/module-env-27.h
index b491b60fbb..e63843f8d6 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -2,3 +2,9 @@
      function should quit.  */
   enum emacs_process_input_result (*process_input) (emacs_env *env)
     EMACS_ATTRIBUTE_NONNULL (1);
+
+  struct timespec (*extract_time) (emacs_env *env, emacs_value value)
+    EMACS_ATTRIBUTE_NONNULL (1);
+
+  emacs_value (*make_time) (emacs_env *env, struct timespec time)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index a39e41afee..44a28fa18f 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -366,6 +366,18 @@ Fmod_test_sleep_until (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->intern (env, "finished");
 }
 
+static emacs_value
+Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                          void *data)
+{
+  assert (nargs == 1);
+  struct timespec time = env->extract_time (env, args[0]);
+  assert (time.tv_nsec >= 0);
+  assert (time.tv_nsec < 1000000000);
+  time.tv_nsec++;
+  return env->make_time (env, time);
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -434,6 +446,7 @@ emacs_module_init (struct emacs_runtime *ert)
   DEFUN ("mod-test-invalid-finalizer", Fmod_test_invalid_finalizer, 0, 0,
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
+  DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 35aaaa64b6..6b986a96e2 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -310,4 +310,29 @@ module--test-assertion
                       'finished))
         (quit)))))
 
+(ert-deftest mod-test-add-nanosecond/valid ()
+  (dolist (input (list
+                  ;; Some realistic examples.
+                  (current-time) (time-to-seconds)
+                  (encode-time 12 34 5 6 7 2019 t)
+                  ;; Various legacy timestamp forms.
+                  '(123 456) '(123 456 789) '(123 456 789 6000)
+                  ;; Corner case: this will result in a nanosecond
+                  ;; value of 1000000000 after addition.  The module
+                  ;; code should handle this correctly.
+                  '(123 65535 999999 999000)
+                  ;; Seconds since the epoch.
+                  123 123.45
+                  ;; New (TICKS . HZ) format.
+                  '(123456789 . 1000000000)))
+    (ert-info ((format "input: %s" input))
+      (should (time-equal-p (mod-test-add-nanosecond input)
+                            (time-add input '(0 0 0 1000)))))))
+
+(ert-deftest mod-test-add-nanosecond/invalid ()
+  (dolist (input '(#x10000000000000000  ; out of range
+                   (123) (123.45 6 7) nil "foo" [1 2]))
+    (ert-info ((format "input: %s" input))
+      (should-error (mod-test-add-nanosecond input)))))
+
 ;;; emacs-module-tests.el ends here
--
2.20.1 (Apple Git-117)


Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Add module functions to convert from and to big integers.

Philipp Stephani
* src/module-env-27.h: Add new module functions to convert big
integers.

* src/emacs-module.c (module_required_bytes)
(module_extract_big_integer, module_make_big_integer): New functions.
(initialize_environment): Use them.
(syms_of_module): Define needed symbols.

* test/data/emacs-module/mod-test.c (signal_memory_full): New helper
function.
(Fmod_test_double): New test function.
(emacs_module_init): Define it.

* test/src/emacs-module-tests.el (mod-test-double): New unit test.

* doc/lispref/internals.texi (Module Values): Document new functions.
---
 doc/lispref/internals.texi        |  26 +++++++
 etc/NEWS                          |   4 ++
 src/emacs-module.c                | 108 ++++++++++++++++++++++++++++++
 src/module-env-27.h               |   9 +++
 test/data/emacs-module/mod-test.c |  48 +++++++++++++
 test/src/emacs-module-tests.el    |   7 ++
 6 files changed, 202 insertions(+)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index c2969d2cd1..17f56907c5 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1382,6 +1382,23 @@ Module Values
 @code{overflow-error}.
 @end deftypefn
 
+@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})
+This function, which is available since Emacs 27, extracts the
+integral value of @var{arg}.  If @var{sign} is not @code{NULL}, it
+stores the sign of @var{arg} (-1, 0, or +1) into @code{*sign}.  The
+magnitude is stored into @var{magnitude} as follows.  If @var{size}
+and @var{magnitude} are bot non-@code{NULL}, then @var{magnitude} must
+point to an array of at least @code{*size} bytes.  If @var{magnitude}
+is large enough to hold the magnitude of @var{arg}, then this function
+writes the magnitude into the @var{magnitude} array in little-endian
+form, stores the number of bytes written into @code{*size}, and
+returns @code{true}.  If @var{magnitude} is not large enough, it
+stores the required size into @code{*size}, signals an error, and
+returns @code{false}.  If @var{size} is not @code{NULL} and
+@var{magnitude} is @code{NULL}, then the function stores the required
+size into @code{*size} and returns @code{true}.
+@end deftypefn
+
 @deftypefn Function double extract_float (emacs_env *@var{env}, emacs_value @var{arg})
 This function returns the value of a Lisp float specified by
 @var{arg}, as a C @code{double} value.
@@ -1456,6 +1473,15 @@ Module Values
 @code{most-positive-fixnum} (@pxref{Integer Basics}).
 @end deftypefn
 
+@deftypefn Function emacs_value make_big_integer (emacs_env *@var{env}, int sign, ptrdiff_t size, unsigned char *magnitude)
+This function, which is available since Emacs 27, takes an
+arbitrary-sized integer argument and returns a corresponding
+@code{emacs_value} object.  The @var{sign} argument gives the sign of
+the return value.  If @var{sign} is nonzero, then @var{magnitude} must
+point to an array of at least @var{size} bytes specifying the
+little-endian magnitude of the return value.
+@end deftypefn
+
 @deftypefn Function emacs_value make_float (emacs_env *@var{env}, double @var{d})
 This function takes a @code{double} argument @var{d} and returns the
 corresponding Emacs floating-point value.
diff --git a/etc/NEWS b/etc/NEWS
index 2534262b62..a0b764217f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1913,6 +1913,10 @@ case.
 ** New module environment functions 'make_time' and 'extract_time' to
 convert between timespec structures and Emacs time values.
 
+** New module environment functions 'make_big_integer' and
+'extract_big_integer' to create and extract arbitrary-size integer
+values.
+
 
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index fc5a912d85..bdc5370978 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -77,6 +77,7 @@ To add a new module function, proceed as follows:
 #include <time.h>
 
 #include "lisp.h"
+#include "bignum.h"
 #include "dynlib.h"
 #include "coding.h"
 #include "keyboard.h"
@@ -748,6 +749,108 @@ module_make_time (emacs_env *env, struct timespec time)
   return lisp_to_value (env, make_lisp_time (time));
 }
 
+/* Return the number of bytes needed to represent U.  */
+
+static ptrdiff_t
+module_required_bytes (EMACS_UINT u)
+{
+  if (u == 0)
+    return 0;
+  for (ptrdiff_t i = 0; i < sizeof u; ++i)
+    if (u <= 0xFFu << (8 * i))
+      return i + 1;
+  return sizeof u;
+}
+
+enum
+{
+  /* Constants for mpz_import and mpz_export.  */
+  module_bigint_order = -1,
+  module_bigint_size = 1,
+  module_bigint_endian = -1,
+  module_bigint_nails = 0,
+};
+
+static bool
+module_extract_big_integer (emacs_env *env, emacs_value value,
+                            int *sign, ptrdiff_t *size,
+                            unsigned char *magnitude)
+{
+  MODULE_FUNCTION_BEGIN (false);
+  Lisp_Object o = value_to_lisp (value);
+  CHECK_INTEGER (o);
+  if (size == NULL && magnitude != NULL)
+    wrong_type_argument (Qnull, Qmagnitude);
+  if (FIXNUMP (o))
+    {
+      EMACS_INT x = XFIXNUM (o);
+      if (sign != NULL)
+        *sign = (0 < x) - (x < 0);
+      if (size != NULL)
+        {
+          eassert (-EMACS_INT_MAX <= x);  /* Verify -x is defined. */
+          EMACS_UINT u = x < 0 ? -x : x;
+          verify (UCHAR_MAX == 0xFF);
+          verify (CHAR_BIT == 8);
+          ptrdiff_t required = module_required_bytes (u);
+          if (magnitude != NULL)
+            {
+              if (*size < required)
+                args_out_of_range_3 (make_int (*size), make_int (required),
+                                     make_int (PTRDIFF_MAX));
+              for (ptrdiff_t i = 0; i < required; ++i)
+                magnitude[i] = (u >> (8 * i)) & 0xFFu;
+            }
+          *size = required;
+        }
+    }
+  else
+    {
+      struct Lisp_Bignum *x = XBIGNUM (o);
+      if (sign != NULL)
+        *sign = mpz_sgn (x->value);
+      if (size != NULL)
+        {
+          /* See the remark at the end of the Info node
+             `(gmp) Integer Import and Export'.  */
+          ptrdiff_t required = (mpz_sizeinbase (x->value, 2) + 7) / 8;
+          if (magnitude != NULL)
+            {
+              if (*size < required)
+                args_out_of_range_3 (make_int (*size), make_int (required),
+                                     make_int (PTRDIFF_MAX));
+              verify (sizeof *magnitude == module_bigint_size);
+              size_t written;
+              mpz_export (magnitude, &written, module_bigint_order,
+                          module_bigint_size, module_bigint_endian,
+                          module_bigint_nails, x->value);
+              eassert (written == required);
+            }
+          *size = required;
+        }
+    }
+  return true;
+}
+
+static emacs_value
+module_make_big_integer (emacs_env *env, int sign, ptrdiff_t size,
+                         const unsigned char *magnitude)
+{
+  MODULE_FUNCTION_BEGIN (NULL);
+  if (sign != 0 && size == 0)
+    wrong_type_argument (Qnatnump, Qsize);
+  if (size != 0 && magnitude == NULL)
+    wrong_type_argument (Qarrayp, Qmagnitude);
+  if (sign == 0)
+    return lisp_to_value (env, make_fixed_natnum (0));
+  verify (sizeof *magnitude == module_bigint_size);
+  mpz_import (mpz[0], size, module_bigint_order, module_bigint_size,
+              module_bigint_endian, module_bigint_nails, magnitude);
+  if (sign < 0)
+    mpz_neg (mpz[0], mpz[0]);
+  return lisp_to_value (env, make_integer_mpz ());
+}
+
 
 /* Subroutines.  */
 
@@ -1153,6 +1256,8 @@ initialize_environment (emacs_env *env, struct emacs_env_private *priv)
   env->process_input = module_process_input;
   env->extract_time = module_extract_time;
   env->make_time = module_make_time;
+  env->extract_big_integer = module_extract_big_integer;
+  env->make_big_integer = module_make_big_integer;
   Vmodule_environments = Fcons (make_mint_ptr (env), Vmodule_environments);
   return env;
 }
@@ -1316,6 +1421,9 @@ syms_of_module (void)
 
   DEFSYM (Qmodule_function_p, "module-function-p");
   DEFSYM (Qtimep, "timep");
+  DEFSYM (Qnull, "null");
+
+  DEFSYM (Qmagnitude, "magnitude");
 
   defsubr (&Smodule_load);
 }
diff --git a/src/module-env-27.h b/src/module-env-27.h
index e63843f8d6..b4ecea2902 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -8,3 +8,12 @@
 
   emacs_value (*make_time) (emacs_env *env, struct timespec time)
     EMACS_ATTRIBUTE_NONNULL (1);
+
+  bool (*extract_big_integer) (emacs_env *env, emacs_value value,
+                               int *sign, ptrdiff_t *size,
+                               unsigned char *magnitude)
+    EMACS_ATTRIBUTE_NONNULL (1);
+
+  emacs_value (*make_big_integer) (emacs_env *env, int sign, ptrdiff_t size,
+                                   const unsigned char *magnitude)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index 44a28fa18f..b0c25717e8 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -329,6 +329,17 @@ signal_errno (emacs_env *env, const char *function)
   env->non_local_exit_signal (env, symbol, data);
 }
 
+static void
+signal_memory_full (emacs_env *env)
+{
+  emacs_value symbol = env->intern (env, "error");
+  const char *message = "Out of memory";
+  emacs_value message_value = env->make_string (env, message, strlen (message));
+  emacs_value data
+    = env->funcall (env, env->intern (env, "list"), 1, &message_value);
+  env->non_local_exit_signal (env, symbol, data);
+}
+
 /* A long-running operation that occasionally calls `should_quit' or
    `process_input'.  */
 
@@ -378,6 +389,42 @@ Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->make_time (env, time);
 }
 
+static emacs_value
+Fmod_test_double (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                  void *data)
+{
+  assert (nargs == 1);
+  emacs_value arg = args[0];
+  ptrdiff_t size;
+  if (!env->extract_big_integer (env, arg, NULL, &size, NULL))
+    return NULL;
+  unsigned char *magnitude = malloc (size + 1);
+  if (magnitude == NULL)
+    {
+      signal_memory_full (env);
+      return NULL;
+    }
+  int sign;
+  emacs_value result = NULL;
+  if (!env->extract_big_integer (env, arg, &sign, &size, magnitude))
+    goto out;
+  unsigned int carry = 0;
+  for (ptrdiff_t i = 0; i < size; ++i)
+    {
+      static_assert (UCHAR_MAX == 0xFF, "UCHAR_MAX != 0xFF");
+      static_assert (CHAR_BIT == 8, "CHAR_BIT != 8");
+      static_assert (UINT_MAX >= 2u * UCHAR_MAX, "unsigned int is too small");
+      unsigned int value = 2u * magnitude[i] + carry;
+      magnitude[i] = value & 0xFF;
+      carry = value >> 8;
+      assert (carry <= 1);
+    }
+  magnitude[size] = carry;
+  result = env->make_big_integer (env, sign, size + 1, magnitude);
+ out: free (magnitude);
+  return result;
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -447,6 +494,7 @@ emacs_module_init (struct emacs_runtime *ert)
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
   DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
+  DEFUN ("mod-test-double", Fmod_test_double, 1, 1, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 6b986a96e2..c160ae50b0 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -335,4 +335,11 @@ module--test-assertion
     (ert-info ((format "input: %s" input))
       (should-error (mod-test-add-nanosecond input)))))
 
+(ert-deftest mod-test-double ()
+  (dolist (input (list 0 1 2 -1 42 12345678901234567890
+                       most-positive-fixnum (1+ most-positive-fixnum)
+                       most-negative-fixnum (1- most-negative-fixnum)))
+    (ert-info ((format "input: %d" input))
+      (should (= (mod-test-double input) (* 2 input))))))
+
 ;;; emacs-module-tests.el ends here
--
2.20.1 (Apple Git-117)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Tue, 23 Apr 2019 15:17:42 +0200
> Cc: Philipp Stephani <[hidden email]>
>
> +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})
> +This function, which is available since Emacs 27, extracts the
> +integral value of @var{arg}.  If @var{sign} is not @code{NULL}, it
> +stores the sign of @var{arg} (-1, 0, or +1) into @code{*sign}.  The
> +magnitude is stored into @var{magnitude} as follows.  If @var{size}
> +and @var{magnitude} are bot non-@code{NULL}, then @var{magnitude} must
> +point to an array of at least @code{*size} bytes.  If @var{magnitude}
> +is large enough to hold the magnitude of @var{arg}, then this function
> +writes the magnitude into the @var{magnitude} array in little-endian
> +form, stores the number of bytes written into @code{*size}, and
> +returns @code{true}.  If @var{magnitude} is not large enough, it
> +stores the required size into @code{*size}, signals an error, and
> +returns @code{false}.  If @var{size} is not @code{NULL} and
> +@var{magnitude} is @code{NULL}, then the function stores the required
> +size into @code{*size} and returns @code{true}.
> +@end deftypefn

I think this text should tell more about how magnitude is put into the
MAGNITUDE array.  I needed to look at the implementation to find the
answer to that question.  Since the caller will have to make sure the
array is "large enough", I think the reader needs to know more about
this than this text reveals.

> +@deftypefn Function emacs_value make_big_integer (emacs_env *@var{env}, int sign, ptrdiff_t size, unsigned char *magnitude)
> +This function, which is available since Emacs 27, takes an
> +arbitrary-sized integer argument and returns a corresponding
> +@code{emacs_value} object.

Same here.  In particular, the text mentions "integer argument", but
it's unclear to what exactly that alludes, as there's no "argument" in
the function's signature as shown.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Paul Eggert
In reply to this post by Philipp Stephani
On 4/23/19 6:17 AM, Philipp Stephani wrote:
> +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})y

This sounds reasonably inconvenient for authors of modules, plus it's a
pain to document. Why not just assume GMP instead? It's pretty unlikely
authors would use anything else for bignums. This should simplify the
code not only on the Emacs side but also on the module side; plus it
should improve performance by avoiding roundtrips through mpz_export and
mpz_import.

If a module author really wanted the array-of-unsigned-char
representation (which they probably wouldn't), you could supply a simple
conversion module to help them do that.

Similarly for make_big_integer.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Philipp Stephani
Am Di., 23. Apr. 2019 um 16:51 Uhr schrieb Paul Eggert <[hidden email]>:

>
> On 4/23/19 6:17 AM, Philipp Stephani wrote:
> > +@deftypefn Function bool extract_big_integer (emacs_env *@var{env}, emacs_value @var{arg}, int *@var{sign}, ptrdiff_t *@var{size}, unsigned char *@var{magnitude})y
>
> This sounds reasonably inconvenient for authors of modules, plus it's a
> pain to document. Why not just assume GMP instead? It's pretty unlikely
> authors would use anything else for bignums. This should simplify the
> code not only on the Emacs side but also on the module side; plus it
> should improve performance by avoiding roundtrips through mpz_export and
> mpz_import.

I considered using GMP. However, it has quite a few downsides:
- It would require making emacs-module.h dependent on GMP, even for
users that don't use big integers. Right now it only depends on
standard C, and I'd like to keep it that way.
- It's unclear how well GMP is supported in other languages. mpz_t is
a weird and unusual type, and other languages might not support it
well in their C interface.
- Other languages tend to have their own bigint support, so I don't
think the advantages of using GMP directly are that big.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

Paul Eggert
In reply to this post by Philipp Stephani
On 4/23/19 6:17 AM, Philipp Stephani wrote:
> +@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
> +This function interprets @var{time} as an Emacs time value and returns
> +the corresponding @code{struct timespec}.  @xref{Time of Day}.
> +@var{time} may not be @code{nil}.

Why not have 'nil' mean the same thing here that it does elsewhere for
Emacs time values, namely, the current time? (That would make the code a
tiny bit faster. :-)

> This function raises an error if

"signals an error"

> +  assert (time.tv_nsec < 1000000000);

2 billion would be safer, to cater to (hypothetical and non-POSIX)
implementations that represent leap seconds with 1 billion <= tv_nsec <
2 billion. That's what timespec_cmp does, anyway.

> +  (dolist (input '(#x10000000000000000  ; out of range
This test will fail to be out of range on (hypothetical) machines where
time_t is 128 bits. How about 1.0e+INF instead? You can test -1.0e+INF
and 0.0e+NaN while you're at it.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Paul Eggert
In reply to this post by Philipp Stephani
On 4/23/19 8:12 AM, Philipp Stephani wrote:
> I considered using GMP. However, it has quite a few downsides:
> - It would require making emacs-module.h dependent on GMP, even for
> users that don't use big integers. Right now it only depends on
> standard C, and I'd like to keep it that way.
> - It's unclear how well GMP is supported in other languages. mpz_t is
> a weird and unusual type, and other languages might not support it
> well in their C interface.
> - Other languages tend to have their own bigint support, so I don't
> think the advantages of using GMP directly are that big.

All true, though Emacs requires GMP anyway (one way or another) and it's
typically faster than the non-GMP approaches used in Python (and I
assume elsewhere).

Some of this depends on the importance of performance and convenience
when communicating between Emacs Lisp and GMP-using modules. If these
are unimportant then the current approach is OK. However, I'm thinking
that at least some users will view them as being important.

Could emacs-module.h expose to the user a GMP-style interface only if
the macro __GNU_MP_VERSION is defined? (Or we can choose our own macro
name if we don't want to require the user to include gmp.h before
emacs-module.h.) That way, users that don't use big integers won't need
to worry about GMP at all.

It might also be good (independently of having a GMP-style interface) to
expose just mp_limb_t to module code - we could give it another name and
put it into emacs-module.h as "typedef XXX bignum_limb_t;" where XXX is
either unsigned int, unsigned long int, or unsigned long long int, as
computed by 'configure'. This could avoid much of the overhead of
converting between GMP's representation and the unsigned char
representation.


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Philipp Stephani
Am Di., 23. Apr. 2019 um 17:48 Uhr schrieb Paul Eggert <[hidden email]>:

>
> On 4/23/19 8:12 AM, Philipp Stephani wrote:
> > I considered using GMP. However, it has quite a few downsides:
> > - It would require making emacs-module.h dependent on GMP, even for
> > users that don't use big integers. Right now it only depends on
> > standard C, and I'd like to keep it that way.
> > - It's unclear how well GMP is supported in other languages. mpz_t is
> > a weird and unusual type, and other languages might not support it
> > well in their C interface.
> > - Other languages tend to have their own bigint support, so I don't
> > think the advantages of using GMP directly are that big.
>
> All true, though Emacs requires GMP anyway (one way or another) and it's
> typically faster than the non-GMP approaches used in Python (and I
> assume elsewhere).

Emacs does require GMP, but module authors might not.

>
> Some of this depends on the importance of performance and convenience
> when communicating between Emacs Lisp and GMP-using modules. If these
> are unimportant then the current approach is OK. However, I'm thinking
> that at least some users will view them as being important.

They are definitely not unimportant, though less important than
robustness and simplicity.
OTOH, the proposed approach isn't really simpler or more robust
either. It's just less dependent on third-party libraries.

>
> Could emacs-module.h expose to the user a GMP-style interface only if
> the macro __GNU_MP_VERSION is defined? (Or we can choose our own macro
> name if we don't want to require the user to include gmp.h before
> emacs-module.h.) That way, users that don't use big integers won't need
> to worry about GMP at all.

This is possible (and indeed required) since we couldn't require GMP
headers unconditionally.
My current approach would be to define a new macro EMACS_MODULE_GMP
and wrap the mpz_t type in a datatype like this:

#ifdef EMACS_MODULE_GMP
struct emacs_mpz { mpz_t value; };
#else
struct emacs_mpz;
#endif

We always need to expose the conversion function, that's required for
ABI compatibility. However, using the approach with a forward-declared
struct (that is never defined) would prevent users that don't have GMP
from using the API.

I'm generally fine with that approach as well.

Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Add conversions to and from struct timespec to module interface.

Philipp Stephani
In reply to this post by Paul Eggert
Time values are a fundamental data type, and such conversions are hard
to implement within modules because of the various forms of time
values in Emacs Lisp.  Adding dedicated conversion functions can
significantly simplify module code dealing with times.

This approach uses nanosecond precision.  While Emacs in theory has
support for higher-precision time values, in practice most languages
and standards, such as POSIX, C, Java, and Go, have settled on
nanosecond-precision integers to represent time.

* src/emacs-module.h.in: Add header for struct timespec.

* src/module-env-27.h: Add module functions for time conversion.

* src/emacs-module.c (module_extract_time, module_make_time): New
functions.
(initialize_environment): Use them.

* test/data/emacs-module/mod-test.c (Fmod_test_add_nanosecond): New
test function.
(emacs_module_init): Define it.

* test/src/emacs-module-tests.el (mod-test-add-nanosecond/valid)
(mod-test-add-nanosecond/nil, mod-test-add-nanosecond/invalid): New
unit tests.

* doc/lispref/internals.texi (Module Values): Document time
conversion functions.
---
 doc/lispref/internals.texi        | 22 ++++++++++++++++++++++
 etc/NEWS                          |  3 +++
 src/emacs-module.c                | 17 +++++++++++++++++
 src/emacs-module.h.in             |  1 +
 src/module-env-27.h               |  6 ++++++
 test/data/emacs-module/mod-test.c | 13 +++++++++++++
 test/src/emacs-module-tests.el    | 28 ++++++++++++++++++++++++++++
 7 files changed, 90 insertions(+)

diff --git a/doc/lispref/internals.texi b/doc/lispref/internals.texi
index 25892d4b57..fb838113bc 100644
--- a/doc/lispref/internals.texi
+++ b/doc/lispref/internals.texi
@@ -1387,6 +1387,15 @@ Module Values
 @var{arg}, as a C @code{double} value.
 @end deftypefn
 
+@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
+This function, which is available since Emacs 27, interprets
+@var{time} as an Emacs time value and returns the corresponding
+@code{struct timespec}.  @xref{Time of Day}.  This function signals an
+error if @var{time} is out of range for @code{struct timespec}.  If
+@var{time} has higher precision than nanoseconds, then this function
+truncates it to nanosecond precision.
+@end deftypefn
+
 @deftypefn Function bool copy_string_contents (emacs_env *@var{env}, emacs_value @var{arg}, char *@var{buf}, ptrdiff_t *@var{len})
 This function stores the UTF-8 encoded text of a Lisp string specified
 by @var{arg} in the array of @code{char} pointed by @var{buf}, which
@@ -1452,6 +1461,19 @@ Module Values
 corresponding Emacs floating-point value.
 @end deftypefn
 
+@deftypefn Function emacs_value make_time (emacs_env *@var{env}, struct timespec @var{time})
+This function, which is available since Emacs 27, takes a @code{struct
+timespec} argument @var{time} and returns the corresponding Emacs
+timestamp.  @xref{Time of Day} for the possible return value formats.
+It is not specified in which timestamp format the time is returned,
+but it is always a valid Emacs timestamp.  The return value is exactly
+the same timestamp as @var{time}: all input values are representable,
+and there is never a loss of precision.  @code{time.tv_sec} and
+@code{time.tv_nsec} can be arbitrary values.  In particular, there's
+no requirement that @var{time} be normalized.  This means that
+@code{time.tv_nsec} doesn't have to be in the range [0, 999999999].
+@end deftypefn
+
 @deftypefn Function emacs_value make_string (emacs_env *@var{env}, const char *@var{str}, ptrdiff_t @var{strlen})
 This function creates an Emacs string from C text string pointed by
 @var{str} whose length in bytes, not including the terminating null
diff --git a/etc/NEWS b/etc/NEWS
index b13ab47768..2534262b62 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1910,6 +1910,9 @@ returns a regexp that never matches anything, which is an identity for
 this operation.  Previously, the empty string was returned in this
 case.
 
+** New module environment functions 'make_time' and 'extract_time' to
+convert between timespec structures and Emacs time values.
+
 
 * Changes in Emacs 27.1 on Non-Free Operating Systems
 
diff --git a/src/emacs-module.c b/src/emacs-module.c
index d7704efcf6..b798789351 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -74,6 +74,7 @@ To add a new module function, proceed as follows:
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <time.h>
 
 #include "lisp.h"
 #include "dynlib.h"
@@ -734,6 +735,20 @@ module_process_input (emacs_env *env)
   return emacs_process_input_continue;
 }
 
+static struct timespec
+module_extract_time (emacs_env *env, emacs_value value)
+{
+  MODULE_FUNCTION_BEGIN ((struct timespec) {0});
+  return lisp_time_argument (value_to_lisp (value));
+}
+
+static emacs_value
+module_make_time (emacs_env *env, struct timespec time)
+{
+  MODULE_FUNCTION_BEGIN (NULL);
+  return lisp_to_value (env, make_lisp_time (time));
+}
+
 
 /* Subroutines.  */
 
@@ -1137,6 +1152,8 @@ initialize_environment (emacs_env *env, struct emacs_env_private *priv)
   env->vec_size = module_vec_size;
   env->should_quit = module_should_quit;
   env->process_input = module_process_input;
+  env->extract_time = module_extract_time;
+  env->make_time = module_make_time;
   Vmodule_environments = Fcons (make_mint_ptr (env), Vmodule_environments);
   return env;
 }
diff --git a/src/emacs-module.h.in b/src/emacs-module.h.in
index 009d1583fe..bfbe226dd9 100644
--- a/src/emacs-module.h.in
+++ b/src/emacs-module.h.in
@@ -22,6 +22,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
 
 #include <stdint.h>
 #include <stddef.h>
+#include <time.h>
 
 #ifndef __cplusplus
 #include <stdbool.h>
diff --git a/src/module-env-27.h b/src/module-env-27.h
index b491b60fbb..e63843f8d6 100644
--- a/src/module-env-27.h
+++ b/src/module-env-27.h
@@ -2,3 +2,9 @@
      function should quit.  */
   enum emacs_process_input_result (*process_input) (emacs_env *env)
     EMACS_ATTRIBUTE_NONNULL (1);
+
+  struct timespec (*extract_time) (emacs_env *env, emacs_value value)
+    EMACS_ATTRIBUTE_NONNULL (1);
+
+  emacs_value (*make_time) (emacs_env *env, struct timespec time)
+    EMACS_ATTRIBUTE_NONNULL (1);
diff --git a/test/data/emacs-module/mod-test.c b/test/data/emacs-module/mod-test.c
index a39e41afee..dbdbfecfe6 100644
--- a/test/data/emacs-module/mod-test.c
+++ b/test/data/emacs-module/mod-test.c
@@ -366,6 +366,18 @@ Fmod_test_sleep_until (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
   return env->intern (env, "finished");
 }
 
+static emacs_value
+Fmod_test_add_nanosecond (emacs_env *env, ptrdiff_t nargs, emacs_value *args,
+                          void *data)
+{
+  assert (nargs == 1);
+  struct timespec time = env->extract_time (env, args[0]);
+  assert (time.tv_nsec >= 0);
+  assert (time.tv_nsec < 2000000000);  /* possible leap second */
+  time.tv_nsec++;
+  return env->make_time (env, time);
+}
+
 /* Lisp utilities for easier readability (simple wrappers).  */
 
 /* Provide FEATURE to Emacs.  */
@@ -434,6 +446,7 @@ emacs_module_init (struct emacs_runtime *ert)
   DEFUN ("mod-test-invalid-finalizer", Fmod_test_invalid_finalizer, 0, 0,
          NULL, NULL);
   DEFUN ("mod-test-sleep-until", Fmod_test_sleep_until, 2, 2, NULL, NULL);
+  DEFUN ("mod-test-add-nanosecond", Fmod_test_add_nanosecond, 1, 1, NULL, NULL);
 
 #undef DEFUN
 
diff --git a/test/src/emacs-module-tests.el b/test/src/emacs-module-tests.el
index 35aaaa64b6..eea4c61165 100644
--- a/test/src/emacs-module-tests.el
+++ b/test/src/emacs-module-tests.el
@@ -310,4 +310,32 @@ module--test-assertion
                       'finished))
         (quit)))))
 
+(ert-deftest mod-test-add-nanosecond/valid ()
+  (dolist (input (list
+                  ;; Some realistic examples.
+                  (current-time) (time-to-seconds)
+                  (encode-time 12 34 5 6 7 2019 t)
+                  ;; Various legacy timestamp forms.
+                  '(123 456) '(123 456 789) '(123 456 789 6000)
+                  ;; Corner case: this will result in a nanosecond
+                  ;; value of 1000000000 after addition.  The module
+                  ;; code should handle this correctly.
+                  '(123 65535 999999 999000)
+                  ;; Seconds since the epoch.
+                  123 123.45
+                  ;; New (TICKS . HZ) format.
+                  '(123456789 . 1000000000)))
+    (ert-info ((format "input: %s" input))
+      (should (time-equal-p (mod-test-add-nanosecond input)
+                            (time-add input '(0 0 0 1000)))))))
+
+(ert-deftest mod-test-add-nanosecond/nil ()
+  (should (<= (float-time (mod-test-add-nanosecond nil))
+              (+ (float-time) 1e-9))))
+
+(ert-deftest mod-test-add-nanosecond/invalid ()
+  (dolist (input '(1.0e+INF 1.0e-INF 0.0e+NaN (123) (123.45 6 7) "foo" [1 2]))
+    (ert-info ((format "input: %s" input))
+      (should-error (mod-test-add-nanosecond input)))))
+
 ;;; emacs-module-tests.el ends here
--
2.20.1 (Apple Git-117)


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Paul Eggert
Thanks, these two patches both look good to me. That was quick work!

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

Eli Zaretskii
In reply to this post by Philipp Stephani
> From: Philipp Stephani <[hidden email]>
> Cc: Philipp Stephani <[hidden email]>
> Date: Tue, 23 Apr 2019 23:32:17 +0200
>
> +@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
> +This function, which is available since Emacs 27, interprets
> +@var{time} as an Emacs time value and returns the corresponding
> +@code{struct timespec}.  @xref{Time of Day}.

I think we need to tell something about 'struct timespec' here.
Ideally, simply describe its members, unless they are too platform
dependent.  This text is for C programmers, who will need to look up
the struct to be able to use this function.

>                                            This function signals an
> +error if @var{time} is out of range for @code{struct timespec}.

Instead of "out of range for", I'd prefer saying "cannot be
represented by", perhaps with an example.  "Out of range" is a
complicated subject when talking about time values, and we shouldn't
assume the reader is an expert on time handling.

>                                                                    If
> +@var{time} has higher precision than nanoseconds, then this function
> +truncates it to nanosecond precision.

If we describe 'struct timespec', this sentence will be redundant, I
think.  (Btw, "truncates" or "rounds", and if the former, why not the
latter?)

> +This function, which is available since Emacs 27, takes a @code{struct
> +timespec} argument @var{time} and returns the corresponding Emacs
> +timestamp.  @xref{Time of Day} for the possible return value formats.
                                 ^
There should be a comma there, or makeinfo will produce a warning.

> +It is not specified in which timestamp format the time is returned,
> +but it is always a valid Emacs timestamp.

I question the wisdom of such an obscurity.  There are no secrets
here, as the source code is available, and I can envision use cases
where the programmer does really need to know the structure of the
returned timestamp.  Why should we make this "unspecified"?

>                                           The return value is exactly
> +the same timestamp as @var{time}: all input values are representable,
> +and there is never a loss of precision.  @code{time.tv_sec} and
> +@code{time.tv_nsec} can be arbitrary values.

Here you reference members of 'struct timespec' without describing the
struct first, which I think is confusing.  One more reason to describe
the struct explcitly

> +@code{time.tv_nsec} doesn't have to be in the range [0, 999999999].
                                                       ^^^^^^^^^^^^^^
This should be in @code, and I think using @dots{} instead of the
comma will look better in print.

> +** New module environment functions 'make_time' and 'extract_time' to
> +convert between timespec structures and Emacs time values.
                                           ^^^^^^^^^^^^^^^^^
"Emacs Lisp time values", I'd say.

Thanks.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

Philipp Stephani
Thanks, I've addressed these comments. Since they were minor, I've
pushed the result to master as commit bffceab633.

Am Mi., 24. Apr. 2019 um 09:21 Uhr schrieb Eli Zaretskii <[hidden email]>:

>
> > From: Philipp Stephani <[hidden email]>
> > Cc: Philipp Stephani <[hidden email]>
> > Date: Tue, 23 Apr 2019 23:32:17 +0200
> >
> > +@deftypefn Function struct timespec extract_time (emacs_env *@var{env}, emacs_value @var{time})
> > +This function, which is available since Emacs 27, interprets
> > +@var{time} as an Emacs time value and returns the corresponding
> > +@code{struct timespec}.  @xref{Time of Day}.
>
> I think we need to tell something about 'struct timespec' here.
> Ideally, simply describe its members, unless they are too platform
> dependent.  This text is for C programmers, who will need to look up
> the struct to be able to use this function.

I've documented the structure members and added a reference to the
libc manual. Fortunately the members are standardized by both C and
Posix.

> >                                                                    If
> > +@var{time} has higher precision than nanoseconds, then this function
> > +truncates it to nanosecond precision.
>
> If we describe 'struct timespec', this sentence will be redundant, I
> think.

Not really, there could be other options (e.g. signaling an error).

>  (Btw, "truncates" or "rounds", and if the former, why not the
> latter?)

I think that's just the behavior of lisp_time_argument. I'll see that
I can add further unit tests and improve the documentation.

>
> > +This function, which is available since Emacs 27, takes a @code{struct
> > +timespec} argument @var{time} and returns the corresponding Emacs
> > +timestamp.  @xref{Time of Day} for the possible return value formats.
>                                  ^
> There should be a comma there, or makeinfo will produce a warning.

I don't see any warning, and I think adding a comma there wouldn't be correct.

>
> > +It is not specified in which timestamp format the time is returned,
> > +but it is always a valid Emacs timestamp.
>
> I question the wisdom of such an obscurity.  There are no secrets
> here, as the source code is available, and I can envision use cases
> where the programmer does really need to know the structure of the
> returned timestamp.  Why should we make this "unspecified"?

To be able to change the format if we wish to do so later.
However, the current (TICKS . HZ) representation seems stable enough,
so I've documented it.

> > +@code{time.tv_nsec} doesn't have to be in the range [0, 999999999].
>                                                        ^^^^^^^^^^^^^^
> This should be in @code, and I think using @dots{} instead of the
> comma will look better in print.

This was meant to be a mathematical closed interval. Since people
might not be familiar with that notation, I've reworded it.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] Add conversions to and from struct timespec to module interface.

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Wed, 24 Apr 2019 13:03:21 +0200
> Cc: Paul Eggert <[hidden email]>, Emacs developers <[hidden email]>,
> Philipp Stephani <[hidden email]>
>
> > > +This function, which is available since Emacs 27, takes a @code{struct
> > > +timespec} argument @var{time} and returns the corresponding Emacs
> > > +timestamp.  @xref{Time of Day} for the possible return value formats.
> >                                  ^
> > There should be a comma there, or makeinfo will produce a warning.
>
> I don't see any warning, and I think adding a comma there wouldn't be correct.

You don't see a warning because latest versions of Texinfo stopped
emitting them.

Thanks for the other fixes.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Eli Zaretskii
In reply to this post by Philipp Stephani
> From: Philipp Stephani <[hidden email]>
> Cc: Philipp Stephani <[hidden email]>
> Date: Tue, 23 Apr 2019 23:32:18 +0200
>
> * src/module-env-27.h: Add new module functions to convert big
> integers.
>
> * src/emacs-module.h.in (emacs_mpz): Define if GMP is available.
>
> * src/emacs-module.c (module_extract_big_integer)
> (module_make_big_integer): New functions.
> (initialize_environment): Use them.
>
> * test/data/emacs-module/mod-test.c (Fmod_test_double): New test
> function.
> (emacs_module_init): Define it.
>
> * test/src/emacs-module-tests.el (mod-test-double): New unit test.
>
> * doc/lispref/internals.texi (Module Values): Document new functions.

These changes break the Emacs build on platforms where GMP is not
available and Emacs uses mini-gmp.c.  In particular, lisp.h amd
emacs-module.c unconditionally define EMACS_MODULE_GMP, which then
causes emacs-module.h include gmp.h, whose defionitions conflict with
mini-gmp.h.

It is also not clear whether the new emacs-module facilities are
compatible with mini-gmp, i.e. whether mini-gmp can support the above
interfaces in emacs-module.  because if they can, we have a problem:
emacs-module.h cannot include mini-gmp.h, AFAIU, where the relevant
structures and macros are defined instead of system-wide gmp.h.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Philipp Stephani
Am Mi., 24. Apr. 2019 um 18:03 Uhr schrieb Eli Zaretskii <[hidden email]>:

>
> > From: Philipp Stephani <[hidden email]>
> > Cc: Philipp Stephani <[hidden email]>
> > Date: Tue, 23 Apr 2019 23:32:18 +0200
> >
> > * src/module-env-27.h: Add new module functions to convert big
> > integers.
> >
> > * src/emacs-module.h.in (emacs_mpz): Define if GMP is available.
> >
> > * src/emacs-module.c (module_extract_big_integer)
> > (module_make_big_integer): New functions.
> > (initialize_environment): Use them.
> >
> > * test/data/emacs-module/mod-test.c (Fmod_test_double): New test
> > function.
> > (emacs_module_init): Define it.
> >
> > * test/src/emacs-module-tests.el (mod-test-double): New unit test.
> >
> > * doc/lispref/internals.texi (Module Values): Document new functions.
>
> These changes break the Emacs build on platforms where GMP is not
> available and Emacs uses mini-gmp.c.  In particular, lisp.h amd
> emacs-module.c unconditionally define EMACS_MODULE_GMP, which then
> causes emacs-module.h include gmp.h, whose defionitions conflict with
> mini-gmp.h.
>
> It is also not clear whether the new emacs-module facilities are
> compatible with mini-gmp, i.e. whether mini-gmp can support the above
> interfaces in emacs-module.  because if they can, we have a problem:
> emacs-module.h cannot include mini-gmp.h, AFAIU, where the relevant
> structures and macros are defined instead of system-wide gmp.h.

Thanks, I'll see whether I can make this work.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Wed, 24 Apr 2019 18:37:53 +0200
> Cc: Philipp Stephani <[hidden email]>, Paul Eggert <[hidden email]>,
> Emacs developers <[hidden email]>
>
> Thanks, I'll see whether I can make this work.

Thanks.

In any case, I think the unconditional inclusion of gmp.h in
emacs-module.c should go, because emacs-module.h already does that,
conditionally.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Philipp Stephani
In reply to this post by Philipp Stephani
Am Mi., 24. Apr. 2019 um 18:37 Uhr schrieb Philipp Stephani
<[hidden email]>:

>
> Am Mi., 24. Apr. 2019 um 18:03 Uhr schrieb Eli Zaretskii <[hidden email]>:
> >
> > > From: Philipp Stephani <[hidden email]>
> > > Cc: Philipp Stephani <[hidden email]>
> > > Date: Tue, 23 Apr 2019 23:32:18 +0200
> > >
> > > * src/module-env-27.h: Add new module functions to convert big
> > > integers.
> > >
> > > * src/emacs-module.h.in (emacs_mpz): Define if GMP is available.
> > >
> > > * src/emacs-module.c (module_extract_big_integer)
> > > (module_make_big_integer): New functions.
> > > (initialize_environment): Use them.
> > >
> > > * test/data/emacs-module/mod-test.c (Fmod_test_double): New test
> > > function.
> > > (emacs_module_init): Define it.
> > >
> > > * test/src/emacs-module-tests.el (mod-test-double): New unit test.
> > >
> > > * doc/lispref/internals.texi (Module Values): Document new functions.
> >
> > These changes break the Emacs build on platforms where GMP is not
> > available and Emacs uses mini-gmp.c.  In particular, lisp.h amd
> > emacs-module.c unconditionally define EMACS_MODULE_GMP, which then
> > causes emacs-module.h include gmp.h, whose defionitions conflict with
> > mini-gmp.h.
> >
> > It is also not clear whether the new emacs-module facilities are
> > compatible with mini-gmp, i.e. whether mini-gmp can support the above
> > interfaces in emacs-module.  because if they can, we have a problem:
> > emacs-module.h cannot include mini-gmp.h, AFAIU, where the relevant
> > structures and macros are defined instead of system-wide gmp.h.
>
> Thanks, I'll see whether I can make this work.

Right now I see 2 options:

1. Don't support these functions at all if GMP isn't available (i.e.
signal an error unconditionally)
2. Add another preprocessor macro that would stop emacs-module.h from
including gmp.h.

(1) seems a bit easier and cleaner, but means that we wouldn't support
these functions if Emacs isn't compiled with GMP support.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Wed, 24 Apr 2019 18:57:40 +0200
> Cc: Paul Eggert <[hidden email]>, Emacs developers <[hidden email]>,
> Philipp Stephani <[hidden email]>
>
> Right now I see 2 options:
>
> 1. Don't support these functions at all if GMP isn't available (i.e.
> signal an error unconditionally)
> 2. Add another preprocessor macro that would stop emacs-module.h from
> including gmp.h.
>
> (1) seems a bit easier and cleaner, but means that we wouldn't support
> these functions if Emacs isn't compiled with GMP support.

I don't think I understand (2): how would you declare GMP-related data
types used in emacs-module.c, if you don't include gmp.h?

And there might be option (3): have emacs-module.h duplicate the
portion of mini-gmp.h that are needed when gmp.h isn't available.
It's a bit inelegant, and maintenance burden, but maybe not grave
enough to decide that wer cannot support bignums without a real GMP.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Philipp Stephani
Am Mi., 24. Apr. 2019 um 19:11 Uhr schrieb Eli Zaretskii <[hidden email]>:

>
> > From: Philipp Stephani <[hidden email]>
> > Date: Wed, 24 Apr 2019 18:57:40 +0200
> > Cc: Paul Eggert <[hidden email]>, Emacs developers <[hidden email]>,
> >       Philipp Stephani <[hidden email]>
> >
> > Right now I see 2 options:
> >
> > 1. Don't support these functions at all if GMP isn't available (i.e.
> > signal an error unconditionally)
> > 2. Add another preprocessor macro that would stop emacs-module.h from
> > including gmp.h.
> >
> > (1) seems a bit easier and cleaner, but means that we wouldn't support
> > these functions if Emacs isn't compiled with GMP support.
>
> I don't think I understand (2): how would you declare GMP-related data
> types used in emacs-module.c, if you don't include gmp.h?

It would require the user (or rather, Emacs) to include gmp.h or
mini-gmp.h before including emacs-module.h. I.e.:

#ifndef EMACS_MODULE_DO_NOT_INCLUDE_GMP_H
#include <gmp.h>
#endif

>
> And there might be option (3): have emacs-module.h duplicate the
> portion of mini-gmp.h that are needed when gmp.h isn't available.
> It's a bit inelegant, and maintenance burden, but maybe not grave
> enough to decide that wer cannot support bignums without a real GMP.

I don't think we should replicate internal data structures of
third-party libraries. It's really ugly.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] Add module functions to convert from and to big integers.

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Wed, 24 Apr 2019 19:15:13 +0200
> Cc: Paul Eggert <[hidden email]>, Emacs developers <[hidden email]>,
> Philipp Stephani <[hidden email]>
>
> > > 1. Don't support these functions at all if GMP isn't available (i.e.
> > > signal an error unconditionally)
> > > 2. Add another preprocessor macro that would stop emacs-module.h from
> > > including gmp.h.
> > >
> > > (1) seems a bit easier and cleaner, but means that we wouldn't support
> > > these functions if Emacs isn't compiled with GMP support.
> >
> > I don't think I understand (2): how would you declare GMP-related data
> > types used in emacs-module.c, if you don't include gmp.h?
>
> It would require the user (or rather, Emacs) to include gmp.h or
> mini-gmp.h before including emacs-module.h. I.e.:
>
> #ifndef EMACS_MODULE_DO_NOT_INCLUDE_GMP_H
> #include <gmp.h>
> #endif

Ah, okay.  But then what would emacs-module.c do with these 2 new APIs
if gmp.h was not included?

> > And there might be option (3): have emacs-module.h duplicate the
> > portion of mini-gmp.h that are needed when gmp.h isn't available.
> > It's a bit inelegant, and maintenance burden, but maybe not grave
> > enough to decide that wer cannot support bignums without a real GMP.
>
> I don't think we should replicate internal data structures of
> third-party libraries. It's really ugly.

mini-gmp.h is not third-party, it's Emacs.

The problem I'm struggling with is how can we defend lack of support
of bignums in modules when Emacs itself does support them?

123