pgsql: Silence compiler warning on win32.

mha@postgresql.org (Magnus Hagander) writes:

pgsql/src/test/regress:
pg_regress.c (r1.57 -> r1.58)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/pg_regress.c?r1=1.57&r2=1.58)

Surely this patch is wrong. It is suppressing, not fixing, a critical
warning about a datatype mismatch.

regards, tom lane

Tom Lane wrote:

mha@postgresql.org (Magnus Hagander) writes:

pgsql/src/test/regress:
pg_regress.c (r1.57 -> r1.58)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/test/regress/pg_regress.c?r1=1.57&r2=1.58)

Surely this patch is wrong. It is suppressing, not fixing, a critical
warning about a datatype mismatch.

You mean the signed vs unsigned part? Other than that, int and dword are
always the same on win32...

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Surely this patch is wrong. It is suppressing, not fixing, a critical
warning about a datatype mismatch.

You mean the signed vs unsigned part? Other than that, int and dword are
always the same on win32...

Hmm, need more caffeine I guess. I was thinking dword == long. But in
any case, I'd feel a lot more comfortable if the patch ifdef'd the
declaration of exit_status to match, rather than forcing a cast of the
pointer value. Just a couple weeks ago I wasted a great deal of time
finding a bug that was created by someone overriding this exact type of
compiler warning with a cast to something that *wasn't* binary
compatible. (It worked fine on the author's machine, of course, but
not so much on one with a different sizeof long...)

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Surely this patch is wrong. It is suppressing, not fixing, a critical
warning about a datatype mismatch.

You mean the signed vs unsigned part? Other than that, int and dword are
always the same on win32...

Hmm, need more caffeine I guess. I was thinking dword == long. But in
any case, I'd feel a lot more comfortable if the patch ifdef'd the
declaration of exit_status to match, rather than forcing a cast of the
pointer value. Just a couple weeks ago I wasted a great deal of time
finding a bug that was created by someone overriding this exact type of
compiler warning with a cast to something that *wasn't* binary
compatible. (It worked fine on the author's machine, of course, but
not so much on one with a different sizeof long...)

Hmm. I looked at that, but that kind of just moves things around.

If i change that variable to be DWORD, it still stuffs it into
statuses[i] three lines further down, which then means that the whole
definition of the function wait_for_tests need to be #ifdefed.

I guess the proper solution in that case is to #define a datatype used
for return codes. Is it really worth that for this, though?

//Magnus

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Hmm, need more caffeine I guess. I was thinking dword == long. But in
any case, I'd feel a lot more comfortable if the patch ifdef'd the
declaration of exit_status to match, rather than forcing a cast of the
pointer value.

Hmm. I looked at that, but that kind of just moves things around.

If i change that variable to be DWORD, it still stuffs it into
statuses[i] three lines further down,

Sure, but that's a plain old assignment that can cope with the two
variables being of different widths, so long as the value to be assigned
fits in both. (And if it doesn't, I trust you'll agree that the code is
broken...) Casting at the call is simply going to misbehave, very
nastily, if somehow the variable isn't of the width the function is
expecting.

My concern here, ultimately, is that a cast used in that fashion is
guaranteed to silence a compiler warning even when the warning is
telling you about serious trouble. Because of that, I consider such a
cast to be bad style, no matter how certain you are that it's okay in
a given case. Any C programmer who's dealt with portability issues will
stop and look twice or three times at it, and so you are distracting and
irritating the reader.

I guess the proper solution in that case is to #define a datatype used
for return codes. Is it really worth that for this, though?

Probably not, although I seem to recall we have done that elsewhere
(pg_ctl maybe?)

regards, tom lane

Tom Lane wrote:

Magnus Hagander <magnus@hagander.net> writes:

Tom Lane wrote:

Hmm, need more caffeine I guess. I was thinking dword == long. But in
any case, I'd feel a lot more comfortable if the patch ifdef'd the
declaration of exit_status to match, rather than forcing a cast of the
pointer value.

Hmm. I looked at that, but that kind of just moves things around.

If i change that variable to be DWORD, it still stuffs it into
statuses[i] three lines further down,

Sure, but that's a plain old assignment that can cope with the two
variables being of different widths, so long as the value to be assigned
fits in both. (And if it doesn't, I trust you'll agree that the code is
broken...) Casting at the call is simply going to misbehave, very
nastily, if somehow the variable isn't of the width the function is
expecting.

Ok. Seems reasonble to change it to a cast in that place instead - will do.

I guess the proper solution in that case is to #define a datatype used
for return codes. Is it really worth that for this, though?

Probably not, although I seem to recall we have done that elsewhere
(pg_ctl maybe?)

Yeah, we have done it in one or two places. I'll just go with the cast
per above for this time.

//Magnus