View Issue Details

IDProjectCategoryView StatusLast Update
0000566LDMud 3.5Implementationpublic2009-01-06 08:23
Reporterzesstra Assigned Tozesstra  
PrioritynormalSeverityminorReproducibilityN/A
Status assignedResolutionopen 
Summary0000566: Replace Code-Defines by static inline functions
DescriptionThere are a lot of macros in the driver for convenience, which evaluate to some value or do some little things and are often called, e.g.
#define mstrsize(s) \
    ((s)->size)
from mstrings.c.

I would like to change these defines to static inline functions. Modern compilers are good at inlining (they would even inline without requesting it by 'inline') and the runtime behaviour is identical to the preprocessor macros.
But functions have (at least) three big advantages:
a) they are visible to the compiler and thus the debugger.
b) for debugging it is possible to disable inlining and/or set breakpoints (which you can't in case of preprocessor macros).
c) type safety: functions have explicit types for their arguments and return values and the compiler can check them, which it can't in case of macros. Additionally it is much easier for the programmer to keep track of types and the code is less error-prone.

Example for c):
If I don't know the type of s->size, I have to do a tedious search for the type and definition of s.
A function: static inline size_t mstrsize(string_t *s) {return s->size;}
is much easier in this respect and the compiler can issue warning if foo in mstrsize(foo) is not a pointer to string_t or if len in len=mstrsize(foo) is of an incompatible type.


I suggest to change all macros, which do not change local variables bit by bit to static inlines functions.


side remark:
even for numerical constants defines are not really needed and have again the disadvantage that the define in not visible in the debugger. If you use 'static const long LIMIT 2' the generated code is _exactly the same_ as if you use '#define LIMIT 2L'. (static ensures that the compiler doesn't emit a symbol LIMIT in the data segment.)
So I would even recommend using consts for numerical constants as well, there is no speed or memory penalty (in modern compilers).
Additional Informationhttp://www.fefe.de/know-your-compiler.pdf
http://www.greenend.org.uk/rjk/2003/03/inline.html
http://gcc.gnu.org/onlinedocs/gcc/Inline.html
TagsNo tags attached.

Relationships

related to 0000559 resolvedzesstra LDMud 3.3 Crash in functionlist() (64 bit issue) 
parent of 0000606 confirmed LDMud 3.6 Bytecode cleanup: exchange macros by functions and use dedicated types 
Not all the children of this issue are yet resolved or closed.

Activities

zesstra

2008-09-09 16:50

administrator   ~0000768

Ah, I just had a look at the linked talk again. FTR: it actually recommends to use only "static" because the compiler will then inline small functions or functions which are called only once anyway. That is probably even true...

Largo

2008-09-09 17:00

reporter   ~0000769

d) expression safety:

BLUBB(foo++)
#define BLUBB(x) ((x) + (x)) vs.
inline int BLUBB(int x) { return x+x; }

2008-09-10 16:15

 

comm.c.diff (3,720 bytes)   
Index: src/comm.c
===================================================================
--- src/comm.c	(Revision 2414)
+++ src/comm.c	(Arbeitskopie)
@@ -394,22 +394,26 @@
   /* Tables with the telnet statemachine handlers.
    */
 
-#define TS_DATA       0
-#define TS_IAC        1
-#define TS_WILL       2
-#define TS_WONT       3
-#define TS_DO         4
-#define TS_DONT       5
-#define TS_SB         6
-#define TS_SB_IAC     7
-#define TS_READY      8
-#define TS_SYNCH      9
-#define TS_INVALID   10
+enum telnet_states {
+  TS_DATA = 0,
+  TS_IAC,
+  TS_WILL,
+  TS_WONT,
+  TS_DO,
+  TS_DONT,
+  TS_SB,
+  TS_SB_IAC,
+  TS_READY,
+  TS_SYNCH,
+  TS_INVALID
+};
 
   /* Telnet states
    */
 
-#define TN_START_VALID(x) ((x & ~1) == TS_SB)
+inline int TN_START_VALID(int x) {
+  return (x & ~TS_IAC) == TS_SB;
+}
 
 
 /* --- Misc --- */
@@ -517,17 +521,14 @@
 #  define s6_addr32 __u6_addr.__u6_addr32
 #endif
 
-#ifndef CREATE_IPV6_MAPPED
+inline void CREATE_IPV6_MAPPED(struct in_addr *v6, int32 v4) {
+  v6->s6_addr32[0] = 0;
+  v6->s6_addr32[1] = 0;
+  v6->s6_addr32[2] = 0x0000ffff;
+  v6->s6_addr32[2] = 0xffff0000;
+  v6->s6_addr32[3] = v4;
+}
 
-#define CREATE_IPV6_MAPPED(v6,v4) \
-    ((v6).s6_addr32[0] = 0, \
-     (v6).s6_addr32[1] = 0, \
-     (v6).s6_addr32[2] = 0x0000ffff, \
-     (v6).s6_addr32[2] = 0xffff0000, \
-     (v6).s6_addr32[3] = v4 )
-
-#endif
-
 /* These are the typical IPv6 structures - we use them transparently.
  *
  * --- arpa/inet.h ---
@@ -2754,7 +2755,7 @@
 #ifndef USE_IPV6
                                 net_addr.s_addr = naddr;
 #else
-                                CREATE_IPV6_MAPPED(net_addr, naddr);
+                                CREATE_IPV6_MAPPED(&net_addr, naddr);
 #endif
                                 add_ip_entry(net_addr, rp+12);
                             }
@@ -6400,7 +6401,7 @@
         h = gethostbyname2(hostname, AF_INET);
         if(h)
         {
-            CREATE_IPV6_MAPPED(addr.sin6_addr, *((u_int32_t *)h->h_addr_list[0]));
+            CREATE_IPV6_MAPPED(&addr.sin6_addr, *((u_int32_t *)h->h_addr_list[0]));
             con = connect(sock, (struct sockaddr *) &addr, sizeof(addr));
         }
     }
@@ -7129,7 +7130,7 @@
 
         if (hp->h_addrtype == AF_INET)
         {
-            CREATE_IPV6_MAPPED(name.sin_addr, (u_int32_t)hp->h_addr_list[0]);
+            CREATE_IPV6_MAPPED(&name.sin_addr, (u_int32_t)hp->h_addr_list[0]);
         }
         name.sin_family = AF_INET6;
 #endif /* USE_IPV6 */
@@ -8315,6 +8316,12 @@
     return sp;
 } /* f_query_mud_port() */
 
+inline void TRANSLATE(char c, int i, int length, string_t *rc, unsigned int bitno) {
+  if (c & (1 << bitno))
+    get_txt(rc)[length++] = (char)(i * 8 + bitno);
+}
+
+
 /*-------------------------------------------------------------------------*/
 static void
 get_charset (svalue_t * sp, p_int mode, char charset[32])
@@ -8385,19 +8392,14 @@
         {
             char c = charset[i];
 
-#define TRANSLATE(bitno) \
-            if (c & (1 << bitno)) \
-                get_txt(rc)[length++] = (char)(i * 8 + bitno);
-
-            TRANSLATE(0);
-            TRANSLATE(1);
-            TRANSLATE(2);
-            TRANSLATE(3);
-            TRANSLATE(4);
-            TRANSLATE(5);
-            TRANSLATE(6);
-            TRANSLATE(7);
-#undef TRANSLATE
+            TRANSLATE(c, i, length, rc, 0);
+            TRANSLATE(c, i, length, rc, 1);
+            TRANSLATE(c, i, length, rc, 2);
+            TRANSLATE(c, i, length, rc, 3);
+            TRANSLATE(c, i, length, rc, 4);
+            TRANSLATE(c, i, length, rc, 5);
+            TRANSLATE(c, i, length, rc, 6);
+            TRANSLATE(c, i, length, rc, 7);
         }
 
         put_string(sp, rc);
comm.c.diff (3,720 bytes)   

2008-09-10 17:39

 

alloc.diff (12,762 bytes)   
Index: src/xalloc.c
===================================================================
--- src/xalloc.c	(Revision 2414)
+++ src/xalloc.c	(Arbeitskopie)
@@ -44,13 +44,38 @@
 typedef struct { unsigned long counter, size; } t_stat;
   /* A counter type for statistics and its functions: */
 
-#define count(a,b)           { a.size+=(b); if ((b)<0) --a.counter; else ++a.counter; }
-#define count_up(a,b)        { a.size+=(b); ++a.counter; }
-#define count_up_n(a,b,c)    { a.size+=(b)*(c); a.counter+=(b); }
-#define count_add(a,b)       { a.size+=(b); }
-#define count_back(a,b)      { a.size-=(b); --a.counter; }
-#define count_back_n(a,b,c)  { a.size-=(b)*(c); a.counter-=(b); }
+inline void count_add(t_stat *a, unsigned int b) {
+  a->size += b;
+}
 
+inline void count(t_stat *a, unsigned int b) {
+  count_add(a, b);
+  if (b < 0)
+    --a->counter;
+  else
+    ++a->counter;
+}
+
+inline void count_up(t_stat *a, unsigned int b) {
+  count_add(a, b);
+  ++a->counter;
+}
+
+inline void count_up_n(t_stat *a, unsigned int b, unsigned int c) {
+  count_add(a, b * c);
+  a->counter += b;
+}
+
+inline void count_back(t_stat *a, unsigned int b) {
+  count_add(a, -b);
+  --a->counter;
+}
+
+inline void count_back_n(t_stat *a, unsigned int b, unsigned int c) {
+  count_add(a, -(b * c));
+  a->counter -= b;
+}
+
 typedef p_uint word_t;
   /* Our 'word' type.
    */
@@ -555,9 +580,9 @@
         p[XM_PC]   = (word_t)inter_pc;
 #endif
 #ifdef NO_MEM_BLOCK_SIZE
-    count_up(xalloc_stat, XM_OVERHEAD_SIZE);
+    count_up(&xalloc_stat, XM_OVERHEAD_SIZE);
 #else
-    count_up(xalloc_stat, mem_block_size(p));
+    count_up(&xalloc_stat, mem_block_size(p));
     if (check_max_malloced())
         return NULL;
 #endif
@@ -576,9 +601,9 @@
     {
         word_t *q = (word_t*)p - XM_OVERHEAD;
 #ifdef NO_MEM_BLOCK_SIZE
-        count_back(xalloc_stat, XM_OVERHEAD_SIZE);
+        count_back(&xalloc_stat, XM_OVERHEAD_SIZE);
 #else
-        count_back(xalloc_stat, mem_block_size(q));
+        count_back(&xalloc_stat, mem_block_size(q));
 #endif
         mem_free(q);
     }
@@ -673,8 +698,8 @@
 #ifndef NO_MEM_BLOCK_SIZE
     if (rc != NULL)
     {
-        count_back(xalloc_stat, old_size);
-        count_up(xalloc_stat, mem_block_size(block));
+        count_back(&xalloc_stat, old_size);
+        count_up(&xalloc_stat, mem_block_size(block));
         if (check_max_malloced())
             return NULL;
     }
@@ -749,8 +774,8 @@
     if (t)
     {
 #ifndef NO_MEM_BLOCK_SIZE
-        count_back(xalloc_stat, old_size);
-        count_up(xalloc_stat, mem_block_size(t));
+        count_back(&xalloc_stat, old_size);
+        count_up(&xalloc_stat, mem_block_size(t));
         if (check_max_malloced())
             return NULL;
 #endif
@@ -1262,7 +1287,7 @@
     result = amalloc(size);
     if (result)
     {
-        count_up(clib_alloc_stat, get_block_size(result));
+        count_up(&clib_alloc_stat, get_block_size(result));
     }
 
     return result;
@@ -1278,7 +1303,7 @@
 {
     if (ptr)
     {
-        count_back(clib_alloc_stat, get_block_size(ptr));
+        count_back(&clib_alloc_stat, get_block_size(ptr));
     }
 
     afree(ptr);
Index: src/slaballoc.c
===================================================================
--- src/slaballoc.c	(Revision 2414)
+++ src/slaballoc.c	(Arbeitskopie)
@@ -729,7 +729,7 @@
     if (q[-M_OVERHEAD] & M_GC_FREE)
     {
         q[-M_OVERHEAD] &= ~M_GC_FREE;
-        count_up(perm_alloc_stat, mem_block_total_size(q));
+        count_up(&perm_alloc_stat, mem_block_total_size(q));
     }
 } /* mem_mark_permanent() */
 
@@ -746,7 +746,7 @@
     if (!(q[-M_OVERHEAD] & M_GC_FREE))
     {
         q[-M_OVERHEAD] |= (M_REF|M_GC_FREE);
-        count_back(perm_alloc_stat, mem_block_total_size(q));
+        count_back(&perm_alloc_stat, mem_block_total_size(q));
     }
 } /* mem_mark_collectable() */
 
@@ -1513,8 +1513,8 @@
 {
     ulog2f("slaballoc: deallocate slab %x [%d]\n", slab, ix);
     slabtable[ix].numSlabs--;
-    count_back(small_slab_stat, SLAB_SIZE(slab, ix));
-    count_back_n(small_free_stat, slabtable[ix].numBlocks, slab->size);
+    count_back(&small_slab_stat, SLAB_SIZE(slab, ix));
+    count_back_n(&small_free_stat, slabtable[ix].numBlocks, slab->size);
 #ifdef MALLOC_EXT_STATISTICS
     extstats[SIZE_INDEX(slab->size)].cur_free -= slabtable[ix].numBlocks;
     extstats[EXTSTAT_SLABS].cur_free--;
@@ -1591,7 +1591,7 @@
     ulog2f(" %d bytes -> [%d]\n", size, ix);
 
     /* Update statistics */
-    count_up(small_alloc_stat,size);
+    count_up(&small_alloc_stat,size);
 
 #ifdef MALLOC_EXT_STATISTICS
     extstats[SIZE_INDEX(size)].num_xalloc++;
@@ -1612,7 +1612,7 @@
 
         block = slab->freeList;
         slab->freeList = BLOCK_NEXT(slab->freeList);
-        count_back(small_free_stat, slab->size);
+        count_back(&small_free_stat, slab->size);
 
 #ifdef MALLOC_EXT_STATISTICS
         extstats[ix].cur_free--;
@@ -1701,7 +1701,7 @@
             }
 
             slabtable[ix].numFreeSlabs--;
-            count_back(small_slab_free_stat, SLAB_SIZE(slab, ix));
+            count_back(&small_slab_free_stat, SLAB_SIZE(slab, ix));
 #ifdef MALLOC_EXT_STATISTICS
             extstats[EXTSTAT_SLABS].cur_free--;
 #endif /* MALLOC_EXT_STATISTICS */
@@ -1733,8 +1733,8 @@
             slab->size = size;
 
             slabtable[ix].numSlabs++;
-            count_up(small_slab_stat, slabSize);
-            count_up_n(small_free_stat, numObjects, size);
+            count_up(&small_slab_stat, slabSize);
+            count_up_n(&small_free_stat, numObjects, size);
 #ifdef MALLOC_EXT_STATISTICS
             extstats[ix].cur_free += numObjects;
             extstats[EXTSTAT_SLABS].num_xalloc++;
@@ -1781,7 +1781,7 @@
 
         MADVISE(block, orig_size);
 
-        count_back(small_free_stat, size);
+        count_back(&small_free_stat, size);
 #ifdef MALLOC_EXT_STATISTICS
         extstats[ix].cur_free--;
 #endif /* MALLOC_EXT_STATISTICS */
@@ -1870,7 +1870,7 @@
     /* It's a small block: put it into the slab's free list */
 
     slab = (mslab_t*)(block - (block[M_SIZE] & M_MASK));
-    count_back(small_alloc_stat, slab->size);
+    count_back(&small_alloc_stat, slab->size);
     ix =  SIZE_INDEX(slab->size);
 
     ulog4f("slaballoc:   -> slab %x [%d], freelist %x, %d free\n"
@@ -1933,7 +1933,7 @@
     slab->freeList = block;
     slab->numAllocated--;
 
-    count_up(small_free_stat, slab->size);
+    count_up(&small_free_stat, slab->size);
 
     /* If this slab is not the fresh slab, handle possible list movements.
      */
@@ -1993,7 +1993,7 @@
                 slabtable[ix].firstFree = slab;
 
                 slabtable[ix].numFreeSlabs++;
-                count_up(small_slab_free_stat, SLAB_SIZE(slab, ix));
+                count_up(&small_slab_free_stat, SLAB_SIZE(slab, ix));
 #ifdef MALLOC_EXT_STATISTICS
                 extstats[EXTSTAT_SLABS].cur_alloc--;
                 extstats[EXTSTAT_SLABS].cur_free++;
@@ -2314,7 +2314,7 @@
     }
 #endif
     p = (struct free_block *)(ptr+M_OVERHEAD);
-    count_back(large_free_stat, p->size);
+    count_back(&large_free_stat, p->size);
 #ifdef MALLOC_EXT_STATISTICS
     extstats[EXTSTAT_LARGE].cur_free--;
 #endif /* MALLOC_EXT_STATISTICS */
@@ -2703,7 +2703,7 @@
                                     * register choice
                                     */
     r = (struct free_block *)(ptr+M_OVERHEAD);
-    count_up(large_free_stat, size);
+    count_up(&large_free_stat, size);
 #ifdef MALLOC_EXT_STATISTICS
     extstats[EXTSTAT_LARGE].cur_free++;
     extstat_update_max(extstats+EXTSTAT_LARGE);
@@ -3344,7 +3344,7 @@
         {
             mark_block(ptr+size);
             *(ptr+size) &= ~M_GC_FREE; /* Hands off, GC! */
-            count_up(large_wasted_stat, (*(ptr+size) & M_MASK) * SINT);
+            count_up(&large_wasted_stat, (*(ptr+size) & M_MASK) * SINT);
         }
         else
 #       endif
@@ -3360,7 +3360,7 @@
     /* The block at ptr is all ours */
 
     mark_block(ptr);
-    count_up(large_alloc_stat, size);
+    count_up(&large_alloc_stat, size);
 #ifdef MALLOC_EXT_STATISTICS
     extstats[EXTSTAT_LARGE].num_xalloc++;
     extstats[EXTSTAT_LARGE].cur_alloc++;
@@ -3388,7 +3388,7 @@
     p = (word_t *) ptr;
     p -= M_OVERHEAD;
     size = p[M_LSIZE];
-    count_back(large_alloc_stat, size);
+    count_back(&large_alloc_stat, size);
 #ifdef MALLOC_EXT_STATISTICS
     extstats[EXTSTAT_LARGE].num_xfree++;
     extstats[EXTSTAT_LARGE].cur_alloc--;
@@ -3460,7 +3460,7 @@
         }
         *heap_start = 2;
         *(heap_start+1) = PREV_BLOCK | M_MASK;
-        count_up(large_wasted_stat, 2*SINT);
+        count_up(&large_wasted_stat, 2*SINT);
         assert_stack_gap();
     }
 
@@ -3468,7 +3468,7 @@
     if ((int)brk((char *)heap_end + size) == -1)
         return NULL;
 
-    count_up(sbrk_stat, size);
+    count_up(&sbrk_stat, size);
     heap_end = (word_t*)((char *)heap_end + size);
     heap_end[-1] = THIS_BLOCK | M_MASK;
     heap_end[-2] = M_MASK;
@@ -3535,7 +3535,7 @@
                 /* We can join with the existing heap */
                 p[overhead] &= ~PREV_BLOCK;
                 overlap = SINT;
-                count_back(large_wasted_stat, overlap);
+                count_back(&large_wasted_stat, overlap);
             }
             else
             {
@@ -3559,7 +3559,7 @@
                 heap_end = (word_t *)(block + size);
                 block -= overhead;
                 overlap = overhead * SINT;
-                count_back(large_wasted_stat, overlap);
+                count_back(&large_wasted_stat, overlap);
             }
             else
             {
@@ -3594,7 +3594,7 @@
                 /* Our block directly follows the one we found */
                 block -= overhead;
                 overlap += overhead * SINT;
-                count_back(large_wasted_stat, overhead * SINT);
+                count_back(&large_wasted_stat, overhead * SINT);
             }
             else
             {
@@ -3619,7 +3619,7 @@
                 /* Our block directly preceedes the next one */
                 *(next+1) &= ~PREV_BLOCK;
                 overlap += overhead * SINT;
-                count_back(large_wasted_stat, overhead * SINT);
+                count_back(&large_wasted_stat, overhead * SINT);
             }
             else
             {
@@ -3630,8 +3630,8 @@
         }
     }
 
-    count_up(sbrk_stat, size);
-    count_up(large_wasted_stat, overhead * SINT);
+    count_up(&sbrk_stat, size);
+    count_up(&large_wasted_stat, overhead * SINT);
 
     *pExtra = overlap;
     return block + SINT;
@@ -3681,7 +3681,7 @@
         start[M_LSIZE] += wsize;
         malloc_increment_size_success++;
         malloc_increment_size_total += (start2 - start) - M_OVERHEAD;
-        count_add(large_alloc_stat, wsize);
+        count_add(&large_alloc_stat, wsize);
 
         return start2+M_LSIZE;
     }
@@ -3699,7 +3699,7 @@
         start[M_LSIZE] += wsize;
         malloc_increment_size_success++;
         malloc_increment_size_total += (start2 - start) - M_OVERHEAD;
-        count_add(large_alloc_stat, wsize);
+        count_add(&large_alloc_stat, wsize);
         return start2+M_LSIZE;
     }
 
@@ -3950,7 +3950,7 @@
         {
             /* Unref'd small blocks are definitely lost */
             success++;
-            count_back(xalloc_stat, slab->size - (T_OVERHEAD * SINT));
+            count_back(&xalloc_stat, slab->size - (T_OVERHEAD * SINT));
             dprintf2(gcollect_outfd, "freeing small block 0x%x (user 0x%x)"
                     , (p_uint)p, (p_uint)(p+M_OVERHEAD));
 #ifdef MALLOC_TRACE
@@ -4007,7 +4007,7 @@
             word_t size2, flags2;
 
             success++;
-            count_back(xalloc_stat, mem_block_size(p+ML_OVERHEAD));
+            count_back(&xalloc_stat, mem_block_size(p+ML_OVERHEAD));
 #if defined(MALLOC_TRACE) || defined(MALLOC_LPC_TRACE)
             dprintf1(gcollect_outfd, "freeing large block 0x%x", (p_uint)p);
 #endif
@@ -4319,7 +4319,7 @@
             while (NULL != (slab = slabtable[ix].firstFree))
             {
                 slabtable[ix].firstFree = slab->next;
-                count_back(small_slab_free_stat, SLAB_SIZE(slab, ix));
+                count_back(&small_slab_free_stat, SLAB_SIZE(slab, ix));
                 free_slab(slab, ix);
             }
             slabtable[ix].numFreeSlabs = 0;
@@ -4343,7 +4343,7 @@
                 else
                     slabtable[ix].firstFree = NULL;
                 slabtable[ix].numFreeSlabs--;
-                count_back(small_slab_free_stat, SLAB_SIZE(slab, ix));
+                count_back(&small_slab_free_stat, SLAB_SIZE(slab, ix));
                 free_slab(slab, ix);
             }
 #ifdef DEBUG_MALLOC_ALLOCS
alloc.diff (12,762 bytes)   

2008-09-11 15:50

 

sprintf.c.diff (14,198 bytes)   
Index: src/sprintf.c
===================================================================
--- src/sprintf.c	(Revision 2414)
+++ src/sprintf.c	(Arbeitskopie)
@@ -133,30 +133,32 @@
 
 typedef unsigned int format_info;
 
-#define INFO_T        0xF
-#define INFO_T_ERROR  0x1
-#define INFO_T_NULL   0x2
-#define INFO_T_LPC    0x3
-#define INFO_T_QLPC   0x4
-#define INFO_T_STRING 0x5
-#define INFO_T_INT    0x6
-#define INFO_T_FLOAT  0x7
+enum format_info_t {
+  INFO_T         =    0xF,
+  INFO_T_ERROR   =    0x1,
+  INFO_T_NULL    =    0x2,
+  INFO_T_LPC     =    0x3,
+  INFO_T_QLPC    =    0x4,
+  INFO_T_STRING  =    0x5,
+  INFO_T_INT     =    0x6,
+  INFO_T_FLOAT   =    0x7,
 
-#define INFO_A          0x30  /* Right alignment */
-#define INFO_A_CENTRE   0x10
-#define INFO_A_LEFT     0x20
-#define INFO_A_JUSTIFY  0x30
+  INFO_A         =   0x30, /* Right alignment */
+  INFO_A_CENTRE  =   0x10,
+  INFO_A_LEFT    =   0x20,
+  INFO_A_JUSTIFY =   0x30,
 
-#define INFO_PP       0xC0
-#define INFO_PP_SPACE 0x40
-#define INFO_PP_PLUS  0x80
+  INFO_PP        =   0xC0,
+  INFO_PP_SPACE  =   0x40,
+  INFO_PP_PLUS   =   0x80,
 
-#define INFO_ARRAY    0x100
-#define INFO_COLS     0x200
-#define INFO_TABLE    0x400
+  INFO_ARRAY     =  0x100,
+  INFO_COLS      =  0x200,
+  INFO_TABLE     =  0x400,
 
-#define INFO_PS_ZERO  0x800
-#define INFO_PS_KEEP  0x1000
+  INFO_PS_ZERO   =  0x800,
+  INFO_PS_KEEP   = 0x1000,
+};
 
 /*-------------------------------------------------------------------------*/
 
@@ -166,27 +168,28 @@
 
 /* The error handling */
 
-#define ERROR(x) (longjmp(st->error_jmp, (x)))
+enum format_err {
+  ERR_ID_NUMBER          =     0xFFFF, /* Mask for the error number */
+  ERR_ARGUMENT           = 0xFFFF0000, /* Mask for the arg number */
 
-#define ERR_ID_NUMBER            0xFFFF /* Mask for the error number */
-#define ERR_ARGUMENT         0xFFFF0000 /* Mask for the arg number */
+  ERR_BUFF_OVERFLOW      =        0x1, /* buffer overflowed */
+  ERR_TO_FEW_ARGS        =        0x2, /* more arguments spec'ed than passed */
+  ERR_INVALID_STAR       =        0x3, /* invalid arg to * */
+  ERR_PREC_EXPECTED      =        0x4, /* expected precision not found */
+  ERR_INVALID_FORMAT_STR =        0x5, /* error in format string */
+  ERR_INCORRECT_ARG      =        0x6, /* invalid arg to %[idcxXs] */
+  ERR_CST_REQUIRES_FS    =        0x7, /* field size not given for c/t */
+  ERR_UNDEFINED_TYPE     =        0x8, /* undefined type found */
+  ERR_QUOTE_EXPECTED     =        0x9, /* expected ' not found */
+  ERR_UNEXPECTED_EOS     =        0xA, /* fs terminated unexpectedly */
+  ERR_NULL_PS            =        0xB, /* pad string is null */
+  ERR_ARRAY_EXPECTED     =        0xC, /* array expected */
+  ERR_NOMEM              =        0xD, /* Out of memory */
+  ERR_SIZE_OVERFLOW      =        0xE, /* Fieldsize/precision numeric overflow */
+};
 
-#define ERR_BUFF_OVERFLOW       0x1     /* buffer overflowed */
-#define ERR_TO_FEW_ARGS         0x2     /* more arguments spec'ed than passed */
-#define ERR_INVALID_STAR        0x3     /* invalid arg to * */
-#define ERR_PREC_EXPECTED       0x4     /* expected precision not found */
-#define ERR_INVALID_FORMAT_STR  0x5     /* error in format string */
-#define ERR_INCORRECT_ARG       0x6     /* invalid arg to %[idcxXs] */
-#define ERR_CST_REQUIRES_FS     0x7     /* field size not given for c/t */
-#define ERR_UNDEFINED_TYPE      0x8     /* undefined type found */
-#define ERR_QUOTE_EXPECTED      0x9     /* expected ' not found */
-#define ERR_UNEXPECTED_EOS      0xA     /* fs terminated unexpectedly */
-#define ERR_NULL_PS             0xB     /* pad string is null */
-#define ERR_ARRAY_EXPECTED      0xC     /* array expected */
-#define ERR_NOMEM               0xD     /* Out of memory */
-#define ERR_SIZE_OVERFLOW       0xE     /* Fieldsize/precision numeric overflow */
+#define ERROR(x) (longjmp(st->error_jmp, (x)))
 
-
 #define ERROR1(e,a)              ERROR((e) | (a)<<16)
 #define EXTRACT_ERR_ARGUMENT(i)  ((i)>>16)
 
@@ -327,59 +330,59 @@
 
 /*-------------------------------------------------------------------------*/
 /* Macros */
-#define ADD_CHAR(x) {\
-    if (st->bpos >= BUFF_SIZE) ERROR(ERR_BUFF_OVERFLOW); \
-    if (x == '\n' && st->sppos != -1) st->bpos = st->sppos; \
-    st->sppos = -1; \
-    st->buff[st->bpos++] = x;\
+inline void ADD_CHAR(fmt_state_t *st, char x) {
+    if (st->bpos >= BUFF_SIZE) ERROR(ERR_BUFF_OVERFLOW);
+    if (x == '\n' && st->sppos != -1) st->bpos = st->sppos;
+    st->sppos = -1;
+    st->buff[st->bpos++] = x;
 }
 
   /* Add character <x> to the buffer.
    */
 
 /*-------------------------------------------------------------------------*/
-#define ADD_STRN(s, n) { \
-    if (st->bpos + n > BUFF_SIZE) ERROR(ERR_BUFF_OVERFLOW); \
-    if (n >= 1 && (s)[0] == '\n' && st->sppos != -1) st->bpos = st->sppos; \
-    st->sppos = -1; \
-    memcpy(st->buff+st->bpos, (s), n);\
-    st->bpos += n; \
+inline void ADD_STRN(fmt_state_t *st, const char *s, size_t n) {
+    if (st->bpos + n > BUFF_SIZE) ERROR(ERR_BUFF_OVERFLOW);
+    if (n >= 1 && s[0] == '\n' && st->sppos != -1) st->bpos = st->sppos;
+    st->sppos = -1;
+    memcpy(st->buff+st->bpos, s, n);
+    st->bpos += n;
 }
 
   /* Add the <n> characters from <s> to the buffer.
    */
 
 /*-------------------------------------------------------------------------*/
-#define ADD_CHARN(c, n) { \
-    /* n must not be negative! */ \
-    if (st->bpos + n > BUFF_SIZE) ERROR(ERR_BUFF_OVERFLOW); \
-    if (n >= 1 && c == '\n' && st->sppos != -1) st->bpos = st->sppos; \
-    st->sppos = -1; \
-    memset(st->buff+st->bpos, c, n); \
-    st->bpos += n; \
+inline void ADD_CHARN(fmt_state_t *st, char c, size_t n) {
+    /* n must not be negative! */
+    if (st->bpos + n > BUFF_SIZE) ERROR(ERR_BUFF_OVERFLOW);
+    if (n >= 1 && c == '\n' && st->sppos != -1) st->bpos = st->sppos;
+    st->sppos = -1;
+    memset(st->buff+st->bpos, c, n);
+    st->bpos += n;
 }
 
   /* Add character <c> <n>-times to the buffer.
    */
 
 /*-------------------------------------------------------------------------*/
-#define ADD_PADDING(pad, N) { \
-    int n = (N); \
-\
-    if (!pad[1]) { \
-        ADD_CHARN(*pad, n) \
-    } else { \
-        int l; \
-\
-        l = strlen(pad); \
-        for (i=0; --n >= 0; ) { \
-            if (pad[i] == '\\') \
-                i++; \
-            ADD_CHAR(pad[i]); \
-            if (++i == l) \
-                i = 0; \
-        } \
-    } \
+inline void ADD_PADDING(fmt_state_t *st, const char *pad, size_t N) {
+    int n = N;
+
+    if (!pad[1]) {
+        ADD_CHARN(st, *pad, n);
+    } else {
+        int i, l;
+
+        l = strlen(pad);
+        for (i=0; --n >= 0; ) {
+            if (pad[i] == '\\')
+                i++;
+            ADD_CHAR(st, pad[i]);
+            if (++i == l)
+                i = 0;
+        }
+    }
 }
 
   /* Add the padding string <pad> to the buffer, repeatedly if necessary,
@@ -1082,7 +1085,6 @@
  */
 
 {
-    int i;
     size_t sppos;
     int num_words;    /* Number of words in the input */
     int num_chars;    /* Number of non-space characters in the input */
@@ -1149,7 +1151,7 @@
         for (mark = pos ; pos < len && str[pos] != ' '; pos++) NOOP;
 
         /* Add the word */
-        ADD_STRN(str+mark, pos - mark);
+        ADD_STRN(st, str+mark, pos - mark);
         num_words--;
 
         if (pos >= len || num_words < 1)
@@ -1169,7 +1171,7 @@
         else /* Randomly add one space */
             padlength = min_spaces + (int)random_number(2);
         sppos = st->bpos;
-        ADD_PADDING(" ", padlength);
+        ADD_PADDING(st, " ", padlength);
         st->sppos = sppos;
         num_spaces -= padlength;
 
@@ -1189,7 +1191,6 @@
  */
 
 {
-    int i;
     size_t sppos;
     Bool is_space_pad;
 
@@ -1206,10 +1207,10 @@
     case INFO_A_JUSTIFY:
     case INFO_A_LEFT:
         /* Also called for the last line of a justified block */
-        ADD_STRN(str, len)
+        ADD_STRN(st, str, len);
         if (is_space_pad)
             sppos = st->bpos;
-        ADD_PADDING(pad, fs - len)
+        ADD_PADDING(st, pad, fs - len);
         if (is_space_pad)
             st->sppos = sppos;
         break;
@@ -1217,16 +1218,16 @@
     case INFO_A_CENTRE:
         if (finfo & INFO_PS_ZERO)
         {
-            ADD_PADDING("0", (fs - len + 1) >> 1)
+            ADD_PADDING(st, "0", (fs - len + 1) >> 1);
         }
         else
         {
-            ADD_PADDING(pad, (fs - len + 1) >> 1)
+            ADD_PADDING(st, pad, (fs - len + 1) >> 1);
         }
-        ADD_STRN(str, len)
+        ADD_STRN(st, str, len);
         if (is_space_pad)
             sppos = st->bpos;
-        ADD_PADDING(pad, (fs - len) >> 1)
+        ADD_PADDING(st, pad, (fs - len) >> 1);
         if (is_space_pad)
             st->sppos = sppos;
         break;
@@ -1236,13 +1237,13 @@
          */
         if (finfo & INFO_PS_ZERO)
         {
-            ADD_PADDING("0", fs - len)
+            ADD_PADDING(st, "0", fs - len);
         }
         else
         {
-            ADD_PADDING(pad, fs - len)
+            ADD_PADDING(st, pad, fs - len);
         }
-        ADD_STRN(str, len)
+        ADD_STRN(st, str, len);
       }
     }
 } /* add_aligned() */
@@ -1403,7 +1404,7 @@
         for (; i < TAB->nocols; i++)
         {
             /* TAB->size is not negative. */
-            ADD_CHARN(' ', done)
+            ADD_CHARN(st, ' ', done);
         }
     }
 
@@ -1661,11 +1662,11 @@
                  */
 
                 if (column_stat == 2)
-                    ADD_CHAR('\n');
+                    ADD_CHAR(st, '\n');
                 column_stat = 0;
                 if (!format_str[fpos])
                     break;
-                ADD_CHAR('\n');
+                ADD_CHAR(st, '\n');
                 st->line_start = st->bpos;
                 continue;
             }
@@ -1673,7 +1674,7 @@
             column_stat = 0; /* If there was a newline pending, it
                               * will be implicitely added now.
                               */
-            ADD_CHAR('\n');
+            ADD_CHAR(st, '\n');
             st->line_start = st->bpos;
 
             /* Handle pending columns and tables */
@@ -1692,7 +1693,7 @@
                             while (*((*temp)->d.col) == ' ')
                                 (*temp)->d.col++;
                         i = (*temp)->start - (st->bpos - st->line_start);
-                        ADD_CHARN(' ', i);
+                        ADD_CHARN(st, ' ', i);
                         column_stat = add_column(st, temp);
                         if (!column_stat)
                             temp = &((*temp)->next);
@@ -1701,19 +1702,19 @@
                     {
                         i = (*temp)->start - (st->bpos - st->line_start);
                         if (i > 0)
-                            ADD_CHARN(' ', i);
+                            ADD_CHARN(st, ' ', i);
                         if (!add_table(st, temp))
                             temp = &((*temp)->next);
                     }
                 } /* while (*temp) */
 
                 if (st->csts || format_str[fpos] == '\n')
-                    ADD_CHAR('\n');
+                    ADD_CHAR(st, '\n');
                 st->line_start = st->bpos;
             } /* while (csts) */
 
             if (column_stat == 2 && format_str[fpos] != '\n')
-                ADD_CHAR('\n');
+                ADD_CHAR(st, '\n');
 
             if (!format_str[fpos])
                 break;
@@ -1726,16 +1727,16 @@
 
             if (format_str[fpos+1] == '%')
             {
-                ADD_CHAR('%');
+                ADD_CHAR(st, '%');
                 fpos++;
                 continue;
             }
 
             if (format_str[fpos+1] == '^')
             {
-                ADD_CHAR('%');
+                ADD_CHAR(st, '%');
                 fpos++;
-                ADD_CHAR('^');
+                ADD_CHAR(st, '^');
                 continue;
             }
 
@@ -1935,7 +1936,7 @@
                     /* never reached... */
                     fprintf(stderr, "%s: (s)printf: INFO_T_NULL.... found.\n"
                                   , get_txt(current_object->name));
-                    ADD_CHAR('%');
+                    ADD_CHAR(st, '%');
                     break;
                   }
 
@@ -2192,7 +2193,7 @@
                         }
                         else
                         {
-                            ADD_STRN(get_txt(carg->u.str), slen)
+                            ADD_STRN(st, get_txt(carg->u.str), slen);
                         }
                     }
                     break;
@@ -2238,7 +2239,7 @@
                     if ((unsigned int)tmpl < fs)
                         add_aligned(st, temp, tmpl, pad, fs, finfo);
                     else
-                        ADD_STRN(temp, tmpl)
+                        ADD_STRN(st, temp, tmpl);
                     break;
                   }
                   else
@@ -2344,14 +2345,14 @@
                              * with leading zeroes: preserve the sign
                              * character in the right place.
                              */
-                            ADD_STRN(temp, 1);
+                            ADD_STRN(st, temp, 1);
                             add_aligned(st, temp+1, tmpl-1, pad, fs-1, finfo);
                         }
                         else
                             add_aligned(st, temp, tmpl, pad, fs, finfo);
                     }
                     else
-                        ADD_STRN(temp, tmpl)
+                        ADD_STRN(st, temp, tmpl);
                     break;
                   }
                 default:        /* type not found */
@@ -2371,10 +2372,10 @@
         } /* if format entry */
 
         /* Nothing to format: just copy the character */
-        ADD_CHAR(format_str[fpos]);
+        ADD_CHAR(st, format_str[fpos]);
     } /* for (fpos=0; 1; fpos++) */
 
-    ADD_CHAR('\0'); /* Terminate the formatted string */
+    ADD_CHAR(st, '\0'); /* Terminate the formatted string */
 
     /* Restore characters */
     while (st->saves)
sprintf.c.diff (14,198 bytes)   

zesstra

2008-12-23 08:19

administrator   ~0000826

Uh, thank you Largo. I didn't even notice that you attached patches here (probably best to attach a note as well). I will have a look during the next days.

zesstra

2008-12-29 06:09

administrator   ~0000835

The patches seem to be fine for me with one small change: inline -> static INLINE to prevent gcc from emitting a stand-alone copy of the function, if it sucessfually inlines it everywhere. (INLINE as Makro for now, until we eventually require (most of) C99 in 3.5.x)
Actually, 'static' alone may be fully sufficient because the compiler usually decides itself if it honors the request for inlining or not. But I guess, it does not harm either as kind of hint for the programmer.

There is another possibility: to declare the function as 'extern inline' in all but one compilation unit, but it is a little more complicated to do so and I am not sure if it is worth the effort.

zesstra

2009-01-06 08:23

administrator   ~0000853

The #defines in bytecode.h are also a good candidate for this, especially in the light that we have to check the types very carefully on LP64 platforms. If we use typed functions the compiler might help us (like in 0000559).

Unfortunately, some of them increment/decrement an argument:
#define STORE_CODE(p,c) (*(p)++ = (c))

A possibility is to use a pointer to pointer as argument:
static INLINE bytecode_t store_code (bytecode_p *p, bytecode_t c) {
    *((*p)++) = c;
    return c;
}
together with a #define STORE_CODE(p,c) store_code (&(p),(c))

Or we can get rid of the macro altogether with something like:
%s/STORE_CODE(\(.*\), \(.*\))/store_code\(\&\(\1\), \(\2\)\)/gc
in vim which I would actually prefer.

Issue History

Date Modified Username Field Change
2008-09-09 16:16 zesstra New Issue
2008-09-09 16:16 zesstra Status new => assigned
2008-09-09 16:16 zesstra Assigned To => zesstra
2008-09-09 16:50 zesstra Note Added: 0000768
2008-09-09 17:00 Largo Note Added: 0000769
2008-09-10 16:15 Largo File Added: comm.c.diff
2008-09-10 17:39 Largo File Added: alloc.diff
2008-09-11 15:50 Largo File Added: sprintf.c.diff
2008-12-23 08:19 zesstra Note Added: 0000826
2008-12-29 06:09 zesstra Note Added: 0000835
2008-12-29 06:15 zesstra Additional Information Updated
2009-01-04 18:10 zesstra Relationship added related to 0000559
2009-01-06 08:23 zesstra Note Added: 0000853
2009-02-08 10:12 zesstra Relationship added parent of 0000606