FreeTDS Development Group

[jklowden AT schemamania.org]

Hi Freddy, 

I have some questions about how tds_sys_iconv() is written.  I'm looking at it
because VS 2005 kicks out warnings for a lot of stuff related to iconv_t, and
before re-writing it, I'd like to understand why certain choices were made:

1.  Why do all the get/put functions e.g. put_utf16le() use int instead of
size_t?  inbytesleft and outbytesleft are size_t*, and il and ol are size_t, 
but
all the little functions take int as their length argument and many check for 
a
negative length.  

2.  This comment:

         *   also we use unsigned to remove required unsigned casts

    Does that refer to warnings we would get in the get/put functions if they
took signed characters instead of unsigned?  
    
3.  Why the function pointer arrays iconv_gets and iconv_puts?  Why not a 
simple
switch statement instead?  The array is referenced only by tds_sys_iconv() and
a switch would be a lot clearer than bit-twiddling the iconv_t value.  

I could understand using function pointers if tds_sys_iconv_open() *set* the
pointers and tds_sys_iconv() *used* them.  As it is, though, 
tds_sys_iconv_open()
hacks two offsets into the iconv_t, and tds_sys_iconv() unpacks them to look 
up
the functions. 

Suppose we did this instead:

        In tds_sys_iconv_open():
        
                struct get_put_pair { 
                        iconv_get_t get;
                        iconv_put_t put; 
                };
                
                get_put_pair *cd = calloc(1, sizeof(get_put_pair));
                
                /* look up names, set members */
                
                /* ... e.g. ... */
                
                if (strcmp(enc_name, "US-ASCII") == 0)
                        cd->get = get_ascii;

                /* ...  */
                
                return cd;

        In tds_sys_iconv():
        
                iconv_get_t get_func = ((struct get_put_pair*)cd)->get;
                iconv_put_t put_func = ((struct get_put_pair*)cd)->put;
        
        
        In tds_sys_iconv_close():
        
                free(cd);
        
That would be much more portable and understandable code, don't you agree?  

I don't even see why we would bother to define iconv_t as void*.  We define it
only for FreeTDS in tdsiconv.h.  It could just as easily be a get_put_pair*.

Here's to hoping SF is back online soon.  

Regards, 

--jkl