[Emacs-diffs] emacs-25 9294978: Prefer signed integer types in module code

[Paul Eggert]

branch: emacs-25
commit 92949781eb0963fd1b25f1eec4e2d72d2c8ae32e
Author: Paul Eggert <address@hidden>
Commit: Paul Eggert <address@hidden>

    Prefer signed integer types in module code
    
    Generally speaking, at the C level the Emacs source code prefers
    signed types like ‘ptrdiff_t’ to unsigned types like ‘size_t’,
    partly to avoid the usual signedness confusion when comparing values.
    Change the module API to follow this convention.
    Use ‘int’ for small values that can’t exceed INT_MAX.
    * modules/mod-test/mod-test.c (Fmod_test_globref_make)
    (Fmod_test_string_a_to_b, Fmod_test_vector_fill)
    (Fmod_test_vector_eq):
    * src/emacs-module.c (struct emacs_value_frame)
    (module_make_global_ref, module_free_global_ref)
    (module_copy_string_contents, module_make_string)
    (module_vec_set, module_vec_get, module_vec_size):
    * src/emacs-module.h (struct emacs_runtime, struct emacs_env_25):
    * src/lread.c (suffix_p):
    Prefer signed to unsigned integer types.
---
 modules/mod-test/mod-test.c |   14 ++++++------
 src/emacs-module.c          |   46 +++++++++++++++---------------------------
 src/emacs-module.h          |   14 ++++++------
 src/lread.c                 |    4 +-
 4 files changed, 33 insertions(+), 45 deletions(-)

diff --git a/modules/mod-test/mod-test.c b/modules/mod-test/mod-test.c
index d1a4ce0..0160da6 100644
--- a/modules/mod-test/mod-test.c
+++ b/modules/mod-test/mod-test.c
@@ -117,7 +117,7 @@ Fmod_test_globref_make (emacs_env *env, int nargs, 
emacs_value args[],
 {
   /* Make a big string and make it global.  */
   char str[26 * 100];
-  for (size_t i = 0; i < sizeof str; i++)
+  for (int i = 0; i < sizeof str; i++)
     str[i] = 'a' + (i % 26);
 
   /* We don't need to null-terminate str.  */
@@ -133,14 +133,14 @@ Fmod_test_string_a_to_b (emacs_env *env, int nargs, 
emacs_value args[],
                         void *data)
 {
   emacs_value lisp_str = args[0];
-  size_t size = 0;
+  ptrdiff_t size = 0;
   char * buf = NULL;
 
   env->copy_string_contents (env, lisp_str, buf, &size);
   buf = malloc (size);
   env->copy_string_contents (env, lisp_str, buf, &size);
 
-  for (size_t i = 0; i + 1 < size; i++)
+  for (ptrdiff_t i = 0; i + 1 < size; i++)
     if (buf[i] == 'a')
       buf[i] = 'b';
 
@@ -191,8 +191,8 @@ Fmod_test_vector_fill (emacs_env *env, int nargs, 
emacs_value args[], void *data
 {
   emacs_value vec = args[0];
   emacs_value val = args[1];
-  size_t size = env->vec_size (env, vec);
-  for (size_t i = 0; i < size; i++)
+  ptrdiff_t size = env->vec_size (env, vec);
+  for (ptrdiff_t i = 0; i < size; i++)
     env->vec_set (env, vec, i, val);
   return env->intern (env, "t");
 }
@@ -205,8 +205,8 @@ Fmod_test_vector_eq (emacs_env *env, int nargs, emacs_value 
args[], void *data)
 {
   emacs_value vec = args[0];
   emacs_value val = args[1];
-  size_t size = env->vec_size (env, vec);
-  for (size_t i = 0; i < size; i++)
+  ptrdiff_t size = env->vec_size (env, vec);
+  for (ptrdiff_t i = 0; i < size; i++)
     if (!env->eq (env, env->vec_get (env, vec, i), val))
         return env->intern (env, "nil");
   return env->intern (env, "t");
diff --git a/src/emacs-module.c b/src/emacs-module.c
index 166f6dd..8992840 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -78,7 +78,7 @@ struct emacs_value_frame
   struct emacs_value_tag objects[value_frame_size];
 
   /* Index of the next free value in `objects'.  */
-  size_t offset;
+  int offset;
 
   /* Pointer to next frame, if any.  */
   struct emacs_value_frame *next;
@@ -263,13 +263,13 @@ module_make_global_ref (emacs_env *env, emacs_value ref)
     {
       Lisp_Object value = HASH_VALUE (h, i);
       eassert (NATNUMP (value));
-      EMACS_UINT refcount = XFASTINT (value);
-      if (refcount >= MOST_POSITIVE_FIXNUM)
+      EMACS_INT refcount = XFASTINT (value) + 1;
+      if (refcount > MOST_POSITIVE_FIXNUM)
         {
           module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
           return NULL;
         }
-      XSETFASTINT (value, refcount + 1);
+      value = make_natnum (refcount);
       set_hash_value_slot (h, i, value);
     }
   else
@@ -297,15 +297,15 @@ module_free_global_ref (emacs_env *env, emacs_value ref)
     {
       Lisp_Object value = HASH_VALUE (h, i);
       eassert (NATNUMP (value));
-      EMACS_UINT refcount = XFASTINT (value);
-      eassert (refcount > 0);
-      if (refcount > 1)
+      EMACS_INT refcount = XFASTINT (value) - 1;
+      if (refcount > 0)
         {
-          XSETFASTINT (value, refcount - 1);
+          value = make_natnum (refcount - 1);
           set_hash_value_slot (h, i, value);
         }
       else
         {
+          eassert (refcount == 0);
           hash_remove_from_table (h, value);
         }
     }
@@ -503,7 +503,7 @@ module_make_float (emacs_env *env, double d)
 
 static bool
 module_copy_string_contents (emacs_env *env, emacs_value value, char *buffer,
-                            size_t *length)
+                            ptrdiff_t *length)
 {
   check_main_thread ();
   eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
@@ -515,7 +515,7 @@ module_copy_string_contents (emacs_env *env, emacs_value 
value, char *buffer,
       return false;
     }
 
-  size_t raw_size = SBYTES (lisp_str);
+  ptrdiff_t raw_size = SBYTES (lisp_str);
 
   /* Emacs internal encoding is more-or-less UTF8, let's assume utf8
      encoded emacs string are the same byte size.  */
@@ -536,7 +536,7 @@ module_copy_string_contents (emacs_env *env, emacs_value 
value, char *buffer,
 }
 
 static emacs_value
-module_make_string (emacs_env *env, const char *str, size_t length)
+module_make_string (emacs_env *env, const char *str, ptrdiff_t length)
 {
   check_main_thread ();
   eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
@@ -609,7 +609,7 @@ module_set_user_finalizer (emacs_env *env, emacs_value uptr,
 }
 
 static void
-module_vec_set (emacs_env *env, emacs_value vec, size_t i, emacs_value val)
+module_vec_set (emacs_env *env, emacs_value vec, ptrdiff_t i, emacs_value val)
 {
   check_main_thread ();
   eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
@@ -633,41 +633,29 @@ module_vec_set (emacs_env *env, emacs_value vec, size_t 
i, emacs_value val)
 }
 
 static emacs_value
-module_vec_get (emacs_env *env, emacs_value vec, size_t i)
+module_vec_get (emacs_env *env, emacs_value vec, ptrdiff_t i)
 {
-  /* Type of ASIZE (lvec) is ptrdiff_t, make sure it fits.  */
-  verify (PTRDIFF_MAX <= SIZE_MAX);
   check_main_thread ();
   eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
-  if (i > MOST_POSITIVE_FIXNUM)
-    {
-      module_non_local_exit_signal_1 (env, Qoverflow_error, Qnil);
-      return NULL;
-    }
   Lisp_Object lvec = value_to_lisp (vec);
   if (! VECTORP (lvec))
     {
       module_wrong_type (env, Qvectorp, lvec);
       return NULL;
     }
-  /* Prevent error-prone comparison between types of different signedness.  */
-  size_t size = ASIZE (lvec);
+  ptrdiff_t size = ASIZE (lvec);
   eassert (size >= 0);
-  if (i >= size)
+  if (! (0 <= i && i < size))
     {
-      if (i > MOST_POSITIVE_FIXNUM)
-       i = (size_t) MOST_POSITIVE_FIXNUM;
       module_args_out_of_range (env, lvec, make_number (i));
       return NULL;
     }
   return lisp_to_value (env, AREF (lvec, i));
 }
 
-static size_t
+static ptrdiff_t
 module_vec_size (emacs_env *env, emacs_value vec)
 {
-  /* Type of ASIZE (lvec) is ptrdiff_t, make sure it fits.  */
-  verify (PTRDIFF_MAX <= SIZE_MAX);
   check_main_thread ();
   eassert (module_non_local_exit_check (env) == emacs_funcall_exit_return);
   Lisp_Object lvec = value_to_lisp (vec);
@@ -947,7 +935,7 @@ void mark_modules (void)
       for (struct emacs_value_frame *frame = &env->priv.storage.initial;
           frame != NULL;
           frame = frame->next)
-        for (size_t i = 0; i < frame->offset; ++i)
+        for (int i = 0; i < frame->offset; ++i)
           mark_object (frame->objects[i].v);
     }
 }
diff --git a/src/emacs-module.h b/src/emacs-module.h
index 2759b1c..a2edf8c 100644
--- a/src/emacs-module.h
+++ b/src/emacs-module.h
@@ -46,7 +46,7 @@ enum emacs_arity { emacs_variadic_function = -2 };
 struct emacs_runtime
 {
   /* Structure size (for version checking).  */
-  size_t size;
+  ptrdiff_t size;
 
   /* Private data; users should not touch this.  */
   struct emacs_runtime_private *private_members;
@@ -82,7 +82,7 @@ enum emacs_funcall_exit
 struct emacs_env_25
 {
   /* Structure size (for version checking).  */
-  size_t size;
+  ptrdiff_t size;
 
   /* Private data; users should not touch this.  */
   struct emacs_env_private *private_members;
@@ -165,11 +165,11 @@ struct emacs_env_25
   bool (*copy_string_contents) (emacs_env *env,
                                 emacs_value value,
                                 char *buffer,
-                                size_t *size_inout);
+                                ptrdiff_t *size_inout);
 
   /* Create a Lisp string from a utf8 encoded string.  */
   emacs_value (*make_string) (emacs_env *env,
-                             const char *contents, size_t length);
+                             const char *contents, ptrdiff_t length);
 
   /* Embedded pointer type.  */
   emacs_value (*make_user_ptr) (emacs_env *env,
@@ -186,12 +186,12 @@ struct emacs_env_25
                              void (*fin) (void *) EMACS_NOEXCEPT);
 
   /* Vector functions.  */
-  emacs_value (*vec_get) (emacs_env *env, emacs_value vec, size_t i);
+  emacs_value (*vec_get) (emacs_env *env, emacs_value vec, ptrdiff_t i);
 
-  void (*vec_set) (emacs_env *env, emacs_value vec, size_t i,
+  void (*vec_set) (emacs_env *env, emacs_value vec, ptrdiff_t i,
                   emacs_value val);
 
-  size_t (*vec_size) (emacs_env *env, emacs_value vec);
+  ptrdiff_t (*vec_size) (emacs_env *env, emacs_value vec);
 };
 
 #ifdef __cplusplus
diff --git a/src/lread.c b/src/lread.c
index f7ce0da..43100d9 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -979,8 +979,8 @@ This uses the variables `load-suffixes' and 
`load-file-rep-suffixes'.  */)
 static bool
 suffix_p (Lisp_Object string, const char *suffix)
 {
-  size_t suffix_len = strlen (suffix);
-  size_t string_len = SBYTES (string);
+  ptrdiff_t suffix_len = strlen (suffix);
+  ptrdiff_t string_len = SBYTES (string);
 
   return string_len >= suffix_len && !strcmp (SSDATA (string) + string_len - 
suffix_len, suffix);
 }