Thread: Elimination of (more or less) all compilation warnings on OSX
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... Then why 99%? I am still seeing exactly 4 warnings in socket.c caused by some deprecated functions in openssl for OSX. I'll come back to that later. The patch is 160kB, so I had to compress it to bypass the ML restrictions. Regards, -- Michael
Attachment
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. 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. Actually, just changing the signature of make_string slashes the number of warnings significantly: -char *make_string(const char *s, ssize_t len, char *buf, size_t bufsize); +char *make_string(const SQLCHAR *s, SQLINTEGER len, char *buf, size_t bufsize); - Heikki
Attachment
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
Attachment
Heikki Linnakangas <hlinnakangas@vmware.com> writes:
> 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..
It definitely used to lead to bugs on some old platforms. It may be
that everybody is more careful nowadays, but the POSIX spec is still
perfectly clear about it:
The c argument is an int, the value of which the application
shall ensure is a character representable as an unsigned char or
equal to the value of the macro EOF. If the argument has any
other value, the behavior is undefined.
I had my old HPUX box hacked up so that gcc would produce "subscript
is a char" warnings for unsafe usage. It seems a bit more difficult to
get modern platforms to do that, though; people stopped exposing the
underlying flag array in the system headers ...
regards, tom lane
On 03/06/2014 06:50 PM, Tom Lane wrote: > Heikki Linnakangas <hlinnakangas@vmware.com> writes: >> 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.. > > It definitely used to lead to bugs on some old platforms. It may be > that everybody is more careful nowadays, but the POSIX spec is still > perfectly clear about it: > > The c argument is an int, the value of which the application > shall ensure is a character representable as an unsigned char or > equal to the value of the macro EOF. If the argument has any > other value, the behavior is undefined. > > I had my old HPUX box hacked up so that gcc would produce "subscript > is a char" warnings for unsafe usage. It seems a bit more difficult to > get modern platforms to do that, though; people stopped exposing the > underlying flag array in the system headers ... A-ha, clever. I bet we could do something similar with modern compilers too. Searching around, I found this trick (from https://stackoverflow.com/questions/4712720/typechecking-macro-arguments-in-c): > #define CHECK_TYPE(var, type) \ > { \ > __typeof(var) *__tmp; \ > __tmp = (type *) NULL; \ > (void) __tmp; /* to avoid "set but not used" warnings */ \ > } The idea is to create a pointer variable that has the same type as the argument, and assign an unsigned char value to it. With -Wpointer-sign, gcc will complain. That's not portable, but that's OK. If we can get some compilers to complain, that's enough. With that, we can do: #ifdef isalnum #undef isalnum #endif #define isalnum(x) ((CHECK_TYPE(x, unsigned char)), isalnum(x)) I did that for all the is* routines, and unsurprisingly there were a bunch of places were we got that wrong in psqlodbc. I'll fix those after we get the minor release out. I'll also try that on the PostgreSQL sources. - Heikki