FreeTDS Development Group

[Frediano Ziglio <freddy77 AT gmail.com>]

2011/1/31  <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.
>

Hi,
  they returns int where <0 on error and >0 on bytes used.
However len/buf_len argument would be better if size_t is used.

> 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?
>

characters is used for shift/vector index operations where signed
would cause some problems (like negative index). Using unsigned
characters solve the problem without many cast in get/put functions.
Is the same problem that cause a core using isspace or similar...

> 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*.
>

Agreed, I think code is so only for historic reason. Perhaps the array
avoid two similar switches. Remember that invalid iconv_t value is
((iconv_t)-1) so tds_sys_iconv_close would be

if (cd != (iconv_t)-1)
  free(cd);

I'd use a struct iconv_internal instead of get_put_pair but is just
question of opinions and style...

> Here's to hoping SF is back online soon.
>

I hope too... from SF blog speaking about CVS "Validation of this data
is going to require several days and we anticipate that this service
will be restored sometime in the later part of week"

freddy77