Re: Elimination of (more or less) all compilation warnings on OSX

On 03/06/2014 03:55 PM, Heikki Linnakangas wrote:
> On 03/06/2014 03:10 PM, Michael Paquier wrote:
>> Hi all,
>>
>> Before beginning my real quest for odbc, please find attached a patch
>> that cleans up 99% of the code warnings I saw on OSX (and actually
>> some of them on Windows). All those warnings are caused by incorrect
>> casts, so fixing them is trivial... But there were quite a lot of
>> them, I think I fixed 100~200 of them...
>
> Those warnings are the reason I added -Wno-pointer-sign to the compiler
> options. Looks like your compiler on OS X doesn't recognize that.
>
> In addition to being ugly, suppressing the warnings with casts can make
> you miss genuine bugs if you change the type of a variable. For example,
> if you have something like this:
>
> char *foo;
> unsigned char *bar;
> ...
> foo = bar;
>
> And you the definition of 'bar' to:
>
> struct mystruct *bar;
>
> With -Wno-pointer-sign, the compiler will still warn you about that. But
> if you had suppressed that by:
>
> foo = (char *) bar;
>
> you will not get a warning.
>
> So I'm generally not in favor of just sprinkling new casts to suppress
> these warnings. In some cases a cast is the best solution, for sure, but
> in general we should rather be consistent with the pointer types we use.
> For example, attached is a patch that fixes up convert.c. It actually
> *removes* one such cast, which is no longer necessary. That's more work
> than just adding casts, but that's what we should do, if anything.

Ok, I just went through all the warnings, attached a patch to fix them
and remove -Wno-pointer-sign. Except for the regression tests; they're
still compiled with -Wno-pointer-sign.

I was going to commit this, but I saw that Hiroshi Saito had just
committed a patch to bump the version number and add release notes for
the minor release. I'm not sure if it would mess with the release
process if I pushed something now, so I didn't.

> We're bound to need some casts, because the ODBC API tends to use
> unsigned char pointers while all the libc string functions use signed.
> But that should be limited to the interface stuff. For example,
> SQLExecDirect has to use an unsigned "SQLCHAR *" in the function
> signature, and PGAPI_ExecDirect is the implementation of that, but in
> PGAPI_ExecDirect, after we convert the SQLCHAR * and length to a string,
> we can deal with it as a signed pointer.

That's pretty much how it went. There are some places where we call
PGAPI_ExecDirect or similar functions internally, with regular C string
and SQL_NTS arguments. Those needed casts. And some functions that took
a "char *" with a length argument, I changed to take "SQLCHAR *" and
"SQLLEN" instead. The general rule is that "char *" means a regular C
null-terminated string, and "SQLCHAR *" + SQLLEN is used when passing a
possibly not-NULL-terminated string with given length.

It's worth noting that isspace, isdigit etc. take an "unsigned char"
argument. The compiler won't warn if you pass a "char", so you have to
be careful with that. In practice, I don't think it will lead to actual
bugs on any real platform and locale, but still..

- Heikki