FreeTDS Development Group

["ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio AT vodafone.com>]

> 
> Frediano recently made a small change to bsqldb.c to address 
> a warning.
> I looked at the change, and I think should treat warnings and casts a
> little more carefully.  Here's the change:
> 
> @@ -228,7 +228,7 @@
>                 /* 'go' or 'GO' separates command batches */
>                 const char *p = query_line;
>                 if (strncasecmp(p, "go", 2) == 0) {
> -                       for (p+=2; isspace(*p); p++) {
> +                       for (p+=2; isspace((unsigned char) *p); p++) {
> 
> And the standards:
> 
> http://www.opengroup.org/onlinepubs/007908799/xsh/isspace.html
> http://www.vmunix.com/~gabor/c/draft.html#6.2.1.1
> 
> isspace(3) takes an int; we passed it a signed char.  ANSI 
> says you can
> use  a signed char wherever an int is called for.  So, 
> isspace(*p) is on
> standard turf.  
> 
> Moreover, the whole point of isspace(3) is to test a char -- almost
> always a signed char, because ASCII defines 7-bit characters. 
>  The code
> as written was completely reasonable, normal, standard C idiom.  
> 

See previous mail. In this case cast is very useful.

> I'm not picking on Frediano.  He's found many bugs and nonportable
> constructions by using different compilers and heap checkers.  But I
> think the tactic of "Oh, there's a warning, let me cast it" is being
> overdone.   Casting away warnings can easily lead to very 
> fragile code,
> because the compiler's warning system is utterly defeated, 
> even when it
> might have been useful.  
> 
> The right remedy IMO is to set the compiler's warning level
> appropriately.  Warnings about standards-compliant, totally safe,
> idiomatic code should be turned off on the command line, not 
> on the code
> line.  
> 
> To check my sense that there's "too much of this", I looked through
> dblib.c.  I didn't find much.  Most of the casts are either made
> necessary by the API definition (e.g., returning BYTE*).  The pointer
> returned by malloc(3) & Co. is cast unless the lvalue is a void* or
> char*.  That's not necessary according to ANSI (because malloc returns
> void*), but once upon a time malloc returned char*.  
> 
> I did find this in dbbylist(), which returns BYTE*:
> 
>                         return (BYTE *) NULL;
> 

Agreed. NULL can always be converted to any pointer without cast.

> Safe, but unnecesary.  At the very end of the file, though, I found a
> bad guy:
> 
> copy_data_to_host_var(DBPROCESS * dbproc, int srctype, const 
> BYTE * src,
> DBINT srclen,
>                                 int desttype, BYTE * dest, DBINT
> destlen,
>                                 int bindtype, DBSMALLINT *indicator)
> {
> ...
>     DBSMALLINT indicator_value = 0;
> ...
>         if (indicator)
>                 *(DBINT *)(indicator) = indicator_value;
> 

This is really bad...

> copy_data_to_host_var() cannot know if indicator points to 
> two bytes or
> four, but it blithely writes a DBINT to a DBSMALLINT's space,
> overwriting two bytes.  No warning, however, because the programmer
> "knew" what he was doing.  
> 
>         if (indicator)
>                 *indicator = indicator_value;
> 
> works just as well, with no downside.  
> 

Yes and not, in this case if works if you change prototype of function
too.

> I'm not the final authority on the question, but I do think 
> less casting
> is better, all in all.  
> 

I like the type safe of C++ where char* cast to void* automatically but
not the opposite. For this reason I always use casts with malloc (recent
ANSI C compilers at least give warnings using other strange casts).

I would avoid casts like (FILE*) NULL... useless... 
$ grep ') \?NULL' `find -name \*.c` | wc -l
  98
(mostly in ctlib)

More about NULL... recently I saw some post on LKML that state that p ==
NULL is not the same of !p... I hope this is true only for kernel...

For some (storic) reasons FreeTDS contains a lot of useless (if not bad)
casts however removed the cases above if cast is not required (for
example to convert data from row to numeric or other type) should be
removed.

freddy77