From 92949781eb0963fd1b25f1eec4e2d72d2c8ae32e Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Thu, 19 Nov 2015 15:01:26 -0800 Subject: [PATCH] Prefer signed integer types in module code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 d1a4ce0d6fa..0160da69d0f 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 166f6ddf93c..8992840307f 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 2759b1cb0f0..a2edf8c0eb3 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 f7ce0daf1fa..43100d9f699 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); }