Re: inlinable functions instead of macros

[Paul Eggert <eggert@cs.ucla.edu>]

On 08/22/2012 07:31 AM, Stefan Monnier wrote:
> It was working simply and effectively

It's simple to put easserts into macros, but as you know this has
problems that make the macros hard to use.  They cannot be invoked
from GDB, which complicates debugging.  More important, callers
sometimes cannot specify arguments with side effects, and this
restriction makes callers error-prone.  We've tried to address some of
the side-effect issues with this hack:

   /* The IDX==IDX tries to detect when the macro argument is side-effecting.  */
   #define ASET(ARRAY, IDX, VAL)	\
	 (eassert ((IDX) == (IDX)),				\
	  eassert ((IDX) >= 0 && (IDX) < ASIZE (ARRAY)),	\
	  XVECTOR (ARRAY)->contents[IDX] = (VAL))

but even this hack doesn't catch the side-effect problem reliably,
and anyway it's better to not introduce further problems like this.

Here's how I'd like to address this issue:

 1. Revert the setters and getters (e.g., bset_filename, BVAR)
    and go back to ordinary field accesses, as described in
    <http://bugs.gnu.org/12215#61>.  This will simplify the problem
    by reducing the number of functions/macros affected.
    We're still working on the details here but it's clear that
    most of these functions will go.

 2. Revert the easserts recently added to Emacs that are contributing
    to this problem.  We went without these easserts for years, and
    there's no particular reason to add them now if they cause problems.

 3. Assuming (1) and (2), only two problematic easserts remain: in
    blv_found and set_blv_found.  Address them with the simple solution
    that Sam Steingold proposed in
    <http://lists.gnu.org/archive/html/emacs-devel/2012-08/msg00641.html>,
    combined with an externally-visible wrapper for GDB's sake.

 4. After 24.3, look into fixing the side-effect problem for ASET and
    other macros too, as described in Bug#11935.  That's a bigger
    project, but it will have other benefits too.

Here's a patch to implement (2) and (3); it assumes the patch
mentioned in (1).  This patch makes Emacs a tiny bit smaller with
the default build, so it should be OK on efficiency grounds.

=== modified file 'src/ChangeLog'
--- src/ChangeLog	2012-08-24 04:43:52 +0000
+++ src/ChangeLog	2012-08-24 21:24:45 +0000
@@ -1,5 +1,17 @@
 2012-08-24  Paul Eggert  <eggert@cs.ucla.edu>
 
+	Report better line numbers when assertions fail (Bug#12215).
+	* alloc.c, lisp.h (LISP_INLINE_EXTERNALLY_VISIBLE): New macro.
+	* buffer.h (buffer_intervals, set_buffer_intervals):
+	* lisp.h (gc_aset, vcopy): Remove recently-introduced easserts, as
+	they're not that useful in practice and they emit bogus line numbers.
+	* lisp.h (eassert, blv_found, set_blv_found):
+	Reimplement in terms of eassert_f, blv_found_f, set_blv_found_f.
+	(eassert_f, blv_found_f, set_blv_found_f): New macros or functions,
+	containing the guts of the old, but which let the caller specify
+	file and line number.
+	(blv_found, set_blv_found): Also define as a function, for GDB.
+
 	Remove setters like bset_keymap and getters like BVAR (Bug#12215).
 	They make the code harder to read, and for now they don't
 	bring anything to the table.  Open-code them instead.

=== modified file 'src/alloc.c'
--- src/alloc.c	2012-08-23 06:49:09 +0000
+++ src/alloc.c	2012-08-24 21:24:45 +0000
@@ -21,6 +21,7 @@
 #include <config.h>
 
 #define LISP_INLINE EXTERN_INLINE
+#define LISP_INLINE_EXTERNALLY_VISIBLE EXTERN_INLINE EXTERNALLY_VISIBLE
 
 #include <stdio.h>
 #include <limits.h>		/* For CHAR_BIT.  */

=== modified file 'src/buffer.h'
--- src/buffer.h	2012-08-24 04:43:52 +0000
+++ src/buffer.h	2012-08-24 21:24:45 +0000
@@ -948,7 +948,6 @@
 BUFFER_INLINE INTERVAL
 buffer_intervals (struct buffer *b)
 {
-  eassert (b->text != NULL);
   return b->text->intervals;
 }
 
@@ -957,7 +956,6 @@
 BUFFER_INLINE void
 set_buffer_intervals (struct buffer *b, INTERVAL i)
 {
-  eassert (b->text != NULL);
   b->text->intervals = i;
 }
 

=== modified file 'src/lisp.h'
--- src/lisp.h	2012-08-24 04:43:52 +0000
+++ src/lisp.h	2012-08-24 21:24:45 +0000
@@ -32,6 +32,7 @@
 INLINE_HEADER_BEGIN
 #ifndef LISP_INLINE
 # define LISP_INLINE INLINE
+# define LISP_INLINE_EXTERNALLY_VISIBLE INLINE
 #endif
 
 /* The ubiquitous max and min macros.  */
@@ -110,8 +111,13 @@
 /* Define an Emacs version of 'assert (COND)', since some
    system-defined 'assert's are flaky.  COND should be free of side
    effects; it may or may not be evaluated.  */
+
+#define eassert(cond) eassert_f (cond, __FILE__, __LINE__)
+
 #ifndef ENABLE_CHECKING
-# define eassert(X) ((void) (0 && (X))) /* Check that X compiles.  */
+  /* Do nothing, but check that args compile. */
+# define eassert_f(cond, file, line) \
+    ((void) (0 && (cond) && (file)[(line) >> 31]))
 #else /* ENABLE_CHECKING */
 
 extern _Noreturn void die (const char *, const char *, int);
@@ -126,10 +132,10 @@
    testing that STRINGP (x) is true, making the test redundant.  */
 extern bool suppress_checking EXTERNALLY_VISIBLE;
 
-# define eassert(cond)						\
+# define eassert_f(cond, file, line)				\
    ((cond) || suppress_checking					\
     ? (void) 0							\
-    : die ("assertion failed: " # cond, __FILE__, __LINE__))
+    : die ("assertion failed: " # cond, file, line))
 #endif /* ENABLE_CHECKING */
 \f
 /* Use the configure flag --enable-check-lisp-object-type to make
@@ -2335,7 +2341,6 @@
 {
   /* Like ASET, but also can be used in the garbage collector:
      sweep_weak_table calls set_hash_key etc. while the table is marked.  */
-  eassert (0 <= idx && idx < (ASIZE (array) & ~ARRAY_MARK_FLAG));
   XVECTOR (array)->contents[idx] = val;
 }
 
@@ -2344,7 +2349,6 @@
 LISP_INLINE void
 vcopy (Lisp_Object v, ptrdiff_t offset, Lisp_Object *args, ptrdiff_t count)
 {
-  eassert (0 <= offset && 0 <= count && offset + count <= ASIZE (v));
   memcpy (XVECTOR (v)->contents + offset, args, count * sizeof *args);
 }
 
@@ -2383,18 +2387,33 @@
 /* Buffer-local (also frame-local) variable access functions.  */
 
 LISP_INLINE int
-blv_found (struct Lisp_Buffer_Local_Value *blv)
+blv_found_f (struct Lisp_Buffer_Local_Value *blv, char const *file, int line)
 {
-  eassert (blv->found == !EQ (blv->defcell, blv->valcell));
+  eassert_f (blv->found == !EQ (blv->defcell, blv->valcell), file, line);
   return blv->found;
 }
+#define blv_found(blv) \
+     blv_found_f (blv, __FILE__, __LINE__)
+LISP_INLINE_EXTERNALLY_VISIBLE int
+(blv_found) (struct Lisp_Buffer_Local_Value *blv)
+{
+  return blv_found (blv);
+}
 
 LISP_INLINE void
-set_blv_found (struct Lisp_Buffer_Local_Value *blv, int found)
+set_blv_found_f (struct Lisp_Buffer_Local_Value *blv, int found,
+		 char const *file, int line)
 {
-  eassert (found == !EQ (blv->defcell, blv->valcell));
+  eassert_f (found == !EQ (blv->defcell, blv->valcell), file, line);
   blv->found = found;
 }
+#define set_blv_found(blv, found) \
+     set_blv_found_f (blv, found, __FILE__, __LINE__)
+LISP_INLINE_EXTERNALLY_VISIBLE void
+(set_blv_found) (struct Lisp_Buffer_Local_Value *blv, int found)
+{
+  set_blv_found (blv, found);
+}
 
 LISP_INLINE Lisp_Object
 blv_value (struct Lisp_Buffer_Local_Value *blv)