Обсуждение: Re: [COMMITTERS] pgsql: Silence compiler warning on win32.

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