Re: [Confuse-devel] CFG_SIMPLE_INT on 64bit machine

[Carlo Marcelo Arenas Belon]

On Thu, Apr 29, 2010 at 06:31:19AM +0200, Martin Hedenfalk wrote:
> It would be better to use a union with proper pointer types for the
> simple value. Patches welcome :-)

Comments welcome ;), and at least gets read of all casts and lets the
compiler do the type checking as suggested.

Some known caveats :

* union initializer would require C99 like support in the compiler
* all non type specific checks are using the pointer type
* use of union type should be ABI compatible but not tested
* cfg_simple_t could be avoided if using instead cfg_value_t* but that
  would require more code changes and would be ABI incompatible

Carlo
---
>From 97ae9e8d7d1771e0a2bb38bc7d719bfe431f7c60 Mon Sep 17 00:00:00 2001
From: Carlo Marcelo Arenas Belon <address@hidden>
Date: Thu, 29 Apr 2010 01:48:31 -0700
Subject: [PATCH] simple: use union instead of void*

---
 src/confuse.c |   34 +++++++++++++++++-----------------
 src/confuse.h |   40 ++++++++++++++++++++++++++--------------
 2 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/src/confuse.c b/src/confuse.c
index be1bf5e..60d77a5 100644
--- a/src/confuse.c
+++ b/src/confuse.c
@@ -210,8 +210,8 @@ DLLIMPORT signed long cfg_opt_getnint(cfg_opt_t *opt, 
unsigned int index)
     assert(opt && opt->type == CFGT_INT);
     if(opt->values && index < opt->nvalues)
         return opt->values[index]->number;
-    else if(opt->simple_value)
-        return *(signed long *)opt->simple_value;
+    else if(opt->simple_value.number)
+        return *opt->simple_value.number;
     else
         return 0;
 }
@@ -232,8 +232,8 @@ DLLIMPORT double cfg_opt_getnfloat(cfg_opt_t *opt, unsigned 
int index)
     assert(opt && opt->type == CFGT_FLOAT);
     if(opt->values && index < opt->nvalues)
         return opt->values[index]->fpnumber;
-    else if(opt->simple_value)
-        return *(double *)opt->simple_value;
+    else if(opt->simple_value.fpnumber)
+        return *opt->simple_value.fpnumber;
     else
         return 0;
 }
@@ -254,8 +254,8 @@ DLLIMPORT cfg_bool_t cfg_opt_getnbool(cfg_opt_t *opt, 
unsigned int index)
     assert(opt && opt->type == CFGT_BOOL);
     if(opt->values && index < opt->nvalues)
         return opt->values[index]->boolean;
-    else if(opt->simple_value)
-        return *(cfg_bool_t *)opt->simple_value;
+    else if(opt->simple_value.boolean)
+        return *opt->simple_value.boolean;
     else
         return cfg_false;
 }
@@ -276,8 +276,8 @@ DLLIMPORT char *cfg_opt_getnstr(cfg_opt_t *opt, unsigned 
int index)
     assert(opt && opt->type == CFGT_STR);
     if(opt->values && index < opt->nvalues)
         return opt->values[index]->string;
-    else if(opt->simple_value)
-        return *(char **)opt->simple_value;
+    else if(opt->simple_value.string)
+        return *opt->simple_value.string;
     else
         return 0;
 }
@@ -297,8 +297,8 @@ DLLIMPORT void *cfg_opt_getnptr(cfg_opt_t *opt, unsigned 
int index)
     assert(opt && opt->type == CFGT_PTR);
     if(opt->values && index < opt->nvalues)
         return opt->values[index]->ptr;
-    else if(opt->simple_value)
-        return *(void **)opt->simple_value;
+    else if(opt->simple_value.ptr)
+        return *opt->simple_value.ptr;
     else
         return 0;
 }
@@ -425,7 +425,7 @@ static void cfg_init_defaults(cfg_t *cfg)
     for(i = 0; cfg->opts[i].name; i++)
     {
         /* libConfuse doesn't handle default values for "simple" options */
-        if(cfg->opts[i].simple_value || is_set(CFGF_NODEFAULT, 
cfg->opts[i].flags))
+        if(cfg->opts[i].simple_value.ptr || is_set(CFGF_NODEFAULT, 
cfg->opts[i].flags))
             continue;
 
         if(cfg->opts[i].type != CFGT_SEC)
@@ -539,10 +539,10 @@ cfg_setopt(cfg_t *cfg, cfg_opt_t *opt, char *value)
 
     assert(cfg && opt);
 
-    if(opt->simple_value)
+    if(opt->simple_value.ptr)
     {
         assert(opt->type != CFGT_SEC);
-        val = (cfg_value_t *)opt->simple_value;
+        val = (cfg_value_t *)opt->simple_value.ptr;
     }
     else
     {
@@ -1238,8 +1238,8 @@ static cfg_value_t *cfg_opt_getval(cfg_opt_t *opt, 
unsigned int index)
 
     assert(index == 0 || is_set(CFGF_LIST, opt->flags));
 
-    if(opt->simple_value)
-        val = (cfg_value_t *)opt->simple_value;
+    if(opt->simple_value.ptr)
+        val = (cfg_value_t *)opt->simple_value.ptr;
     else
     {
         if(is_set(CFGF_RESET, opt->flags))
@@ -1493,9 +1493,9 @@ DLLIMPORT void cfg_opt_print_indent(cfg_opt_t *opt, FILE 
*fp, int indent)
         {
             cfg_indent(fp, indent);
             /* comment out the option if is not set */
-            if(opt->simple_value)
+            if(opt->simple_value.ptr)
             {
-                if(opt->type == CFGT_STR && *((char **)opt->simple_value) == 0)
+                if(opt->type == CFGT_STR && *opt->simple_value.string == 0)
                     fprintf(fp, "# ");
             }
             else
diff --git a/src/confuse.h b/src/confuse.h
index 69215b1..8b5746b 100644
--- a/src/confuse.h
+++ b/src/confuse.h
@@ -97,6 +97,7 @@ typedef enum cfg_type_t cfg_type_t;
 #define CFG_PARSE_ERROR 1
 
 typedef union cfg_value_t cfg_value_t;
+typedef union cfg_simple_t cfg_simple_t;
 typedef struct cfg_opt_t cfg_opt_t;
 typedef struct cfg_t cfg_t;
 typedef struct cfg_defvalue_t cfg_defvalue_t;
@@ -237,6 +238,17 @@ union cfg_value_t {
     void *ptr;              /**< user-defined value */
 };
 
+/** Data structure holding the pointer to a user provided variable
+ *  defined with CFG_SIMPLE_*
+ */
+union cfg_simple_t {
+    long int *number;
+    double *fpnumber;
+    cfg_bool_t *boolean;
+    char **string;
+    void **ptr;
+};
+
 /** Data structure holding the default value given by the
  *  initialization macros.
  */
@@ -263,7 +275,7 @@ struct cfg_opt_t {
     cfg_opt_t *subopts;     /**< Suboptions (only applies to sections) */
     cfg_defvalue_t def;     /**< Default value */
     cfg_func_t func;        /**< Function callback for CFGT_FUNC options */
-    void *simple_value;     /**< Pointer to user-specified variable to
+    cfg_simple_t simple_value;     /**< Pointer to user-specified variable to
                              * store simple values (created with the
                              * CFG_SIMPLE_* initializers) */
     cfg_callback_t parsecb; /**< Value parsing callback function */
@@ -277,9 +289,9 @@ extern const char __export confuse_version[];
 extern const char __export confuse_author[];
 
 #define __CFG_STR(name, def, flags, svalue, cb) \
-  {name,CFGT_STR,0,0,flags,0,{0,0,cfg_false,def,0},0,svalue,cb,0,0,0}
+  {name,CFGT_STR,0,0,flags,0,{0,0,cfg_false,def,0},0,{.string=svalue},cb,0,0,0}
 #define __CFG_STR_LIST(name, def, flags, svalue, cb) \
-  {name,CFGT_STR,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,svalue,cb,0,0,0}
+  {name,CFGT_STR,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,{.string=svalue},cb,0,0,0}
 
 /** Initialize a string option
  */
@@ -358,9 +370,9 @@ extern const char __export confuse_author[];
 
 
 #define __CFG_INT(name, def, flags, svalue, cb) \
-  {name,CFGT_INT,0,0,flags,0,{def,0,cfg_false,0,0},0,svalue,cb,0,0,0}
+  {name,CFGT_INT,0,0,flags,0,{def,0,cfg_false,0,0},0,{.number=svalue},cb,0,0,0}
 #define __CFG_INT_LIST(name, def, flags, svalue, cb) \
-  {name,CFGT_INT,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,svalue,cb,0,0,0}
+  {name,CFGT_INT,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,{.number=svalue},cb,0,0,0}
 
 /** Initialize an integer option
  */
@@ -391,9 +403,9 @@ extern const char __export confuse_author[];
 
 
 #define __CFG_FLOAT(name, def, flags, svalue, cb) \
-  {name,CFGT_FLOAT,0,0,flags,0,{0,def,cfg_false,0,0},0,svalue,cb,0,0,0}
+  
{name,CFGT_FLOAT,0,0,flags,0,{0,def,cfg_false,0,0},0,{.fpnumber=svalue},cb,0,0,0}
 #define __CFG_FLOAT_LIST(name, def, flags, svalue, cb) \
-  {name,CFGT_FLOAT,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,svalue,cb,0,0,0}
+  {name,CFGT_FLOAT,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,{.fpnumber=svalue},cb,0,0,0}
 
 /** Initialize a floating point option
  */
@@ -424,9 +436,9 @@ extern const char __export confuse_author[];
 
 
 #define __CFG_BOOL(name, def, flags, svalue, cb) \
-  {name,CFGT_BOOL,0,0,flags,0,{0,0,def,0,0},0,svalue,cb,0,0,0}
+  {name,CFGT_BOOL,0,0,flags,0,{0,0,def,0,0},0,{.boolean=svalue},cb,0,0,0}
 #define __CFG_BOOL_LIST(name, def, flags, svalue, cb) \
-  {name,CFGT_BOOL,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,svalue,cb,0,0,0}
+  {name,CFGT_BOOL,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,{.boolean=svalue},cb,0,0,0}
 
 /** Initialize a boolean option
  */
@@ -468,7 +480,7 @@ extern const char __export confuse_author[];
  *
  */
 #define CFG_SEC(name, opts, flags) \
-  {name,CFGT_SEC,0,0,flags,opts,{0,0,cfg_false,0,0},0,0,0,0,0,0}
+  {name,CFGT_SEC,0,0,flags,opts,{0,0,cfg_false,0,0},0,{0},0,0,0,0}
 
 
 
@@ -479,13 +491,13 @@ extern const char __export confuse_author[];
  * @see cfg_func_t
  */
 #define CFG_FUNC(name, func) \
-  {name,CFGT_FUNC,0,0,CFGF_NONE,0,{0,0,cfg_false,0,0},func,0,0,0,0,0}
+  {name,CFGT_FUNC,0,0,CFGF_NONE,0,{0,0,cfg_false,0,0},func,{0},0,0,0,0}
 
 
 #define __CFG_PTR(name, def, flags, svalue, parsecb, freecb) \
-  {name,CFGT_PTR,0,0,flags,0,{0,0,cfg_false,0,def},0,svalue,parsecb,0,0,freecb}
+  
{name,CFGT_PTR,0,0,flags,0,{0,0,cfg_false,0,def},0,{.ptr=svalue},parsecb,0,0,freecb}
 #define __CFG_PTR_LIST(name, def, flags, svalue, parsecb, freecb) \
-  {name,CFGT_PTR,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,svalue,parsecb,0,0,freecb}
+  {name,CFGT_PTR,0,0,flags | 
CFGF_LIST,0,{0,0,cfg_false,0,def},0,{.ptr=svalue},parsecb,0,0,freecb}
 
 /** Initialize a user-defined option
  *
@@ -515,7 +527,7 @@ extern const char __export confuse_author[];
  * the option list.
  */
 #define CFG_END() \
-   {0,CFGT_NONE,0,0,CFGF_NONE,0,{0,0,cfg_false,0,0},0,0,0,0,0,0}
+   {0,CFGT_NONE,0,0,CFGF_NONE,0,{0,0,cfg_false,0,0},0,{0},0,0,0,0}
 
 
 
-- 
1.7.0.4