FreeTDS Development Group

["Lowden, James K" <LowdenJK AT bernstein.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.  

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;

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;

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.  

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

--jkl



-----------------------------------------
The information contained in this transmission may contain privileged and 
confidential information and is intended only for the use of the person(s) 
named above. If you are not the intended recipient, or an employee or agent 
responsible for delivering this message to the intended recipient, any 
review, dissemination, distribution or duplication of this communication is 
strictly prohibited. If you are not the intended recipient, please contact 
the sender immediately by reply e-mail and destroy all copies of the original 
message. Please note that we do not accept account orders and/or instructions 
by e-mail, and therefore will not be responsible for carrying out such orders 
and/or instructions.
If you, as the intended recipient of this message, the purpose of which is to 
inform and update our clients, prospects and consultants of developments 
relating to our services and products, would not like to receive further 
e-mail correspondence from the sender, please "reply" to the sender 
indicating your wishes.  In the U.S.: 1345 Avenue of the Americas, New York, 
NY 10105.