Thread: ECPG patch causes warning
Hi! The ecpg patch at http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=2f567552 causes a compile warning on win64 (andi think win32, but I didn't recheck that). Specifically, line 140 of typename.c has: return (-type); Where type is of type Oid, which is unsigned. This causes the warning: .\src\interfaces\ecpg\ecpglib\typename.c(140): warningC4146: unary minus operator applied to unsigned type, result still unsigned I think that line tries to fix that problem, but either it does it wrong, or just in a way that MSVC can't deal with. I think you need to cast it to a signed type as well? -- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes: > The ecpg patch at > http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=2f567552 > causes a compile warning on win64 (andi think win32, but I didn't > recheck that). Specifically, line 140 of typename.c has: > return (-type); > Where type is of type Oid, which is unsigned. I think that the compiler has caught an actual mistake here. It looks to me like the patch is attempting to use a 'negative' Oid to signal a problem, and that simply is going to break as soon as the Oid counter runs past 2G. Perhaps InvalidOid is the thing to use here? I did not look at the call sites though. regards, tom lane
Tom Lane írta: > Magnus Hagander <magnus@hagander.net> writes: > >> The ecpg patch at >> http://git.postgresql.org/gitweb?p=postgresql.git;a=commit;h=2f567552 >> causes a compile warning on win64 (andi think win32, but I didn't >> recheck that). Specifically, line 140 of typename.c has: >> return (-type); >> > > >> Where type is of type Oid, which is unsigned. >> > > I think that the compiler has caught an actual mistake here. > Yes, it's a mistake, but not an actual bug. The intent was to be able to catch unhandled cases in the application, just as in ecpg_dynamic_type(). The fix for sqlda_dynamic_type() is to use the same cast: return -(int) type; Should I send a patch for this? > It looks to me like the patch is attempting to use a 'negative' > Oid to signal a problem, and that simply is going to break as soon > as the Oid counter runs past 2G. Perhaps InvalidOid is the thing > to use here? I did not look at the call sites though. > > regards, tom lane > > -- Bible has answers for everything. Proof: "But let your communication be, Yea, yea; Nay, nay: for whatsoever is more than these cometh of evil." (Matthew 5:37) - basics of digital technology. "May your kingdom come" - superficial description of plate tectonics ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH http://www.postgresql.at/
Boszormenyi Zoltan <zb@cybertec.at> writes:
> Tom Lane �rta:
>> I think that the compiler has caught an actual mistake here.
> Yes, it's a mistake, but not an actual bug.
> The intent was to be able to catch unhandled
> cases in the application, just as in ecpg_dynamic_type().
> The fix for sqlda_dynamic_type() is to use the same cast:
> return -(int) type;
> Should I send a patch for this?
You should send a patch to *get rid of it*. Putting in a cast only
hides the fact that this logic will break on OIDs above 2G.
regards, tom lane
Tom Lane írta:
> Boszormenyi Zoltan <zb@cybertec.at> writes:
>
>> Tom Lane írta:
>>
>>> I think that the compiler has caught an actual mistake here.
>>>
>
>
>> Yes, it's a mistake, but not an actual bug.
>> The intent was to be able to catch unhandled
>> cases in the application, just as in ecpg_dynamic_type().
>> The fix for sqlda_dynamic_type() is to use the same cast:
>>
>
>
>> return -(int) type;
>>
>
>
>> Should I send a patch for this?
>>
>
> You should send a patch to *get rid of it*. Putting in a cast only
> hides the fact that this logic will break on OIDs above 2G.
>
As would ecpg_dynamic_type(), then. :-(
You wrote previously:
> Perhaps InvalidOid is the thing
> to use here?
Perhaps InvalidOid wouldn't be the best choice to return,
because this function returns int, not Oid. InvalidOid equals
to ECPGt_char. Hm... it would be hiding the failure in
a good way, as the type returned couldn't be mapped to
any ECPGt_* type, and the value would be returned in
a string anyway. We can use ECPGt_char for the unhandled case.
Another way (to lose only twenty-something values from 4GB)
would be to introduce ECPGt_max and
return (ECPGt_max + type)
to have mapping for a large amount of type Oids.
There would be a drawback in this case, when
a new type might get added to ECPG, and the value of
ECPGt_max changes. An Oid -> int mapping has the
advantage of being able to catch non-standard types,
but libecpg and the app may have different opinion about
the value of ECPGt_max, as the app may be compiled
against a different version of libecpg in a theoretical future.
I think that leaves the first choice.
Patch is attached.
Best regards,
Zoltán Böszörményi
--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/
Attachment
On Sun, Jan 10, 2010 at 07:16:59PM +0100, Boszormenyi Zoltan wrote: > As would ecpg_dynamic_type(), then. :-( My guess is that this function is fine when returning InvalidOid = 0. AFAICT it is supposed to fill an integer with the SQL3 type code which seems to start with 1 too. So I will change this one to return 0. > Perhaps InvalidOid wouldn't be the best choice to return, > because this function returns int, not Oid. InvalidOid equals > to ECPGt_char. Hm... it would be hiding the failure in No, ECPGt_char is set to 1. > a good way, as the type returned couldn't be mapped to > any ECPGt_* type, and the value would be returned in > a string anyway. We can use ECPGt_char for the unhandled case. The question is, do we want to catch the unhandled case or shall we assume a string? Just tell me and I'll commit. Looking at the usage of sqlda_dynamic_type again we would run into this situation even earlier as the return value then is stort in a short int because that's what the other sqlda deffinitions use too. Therefore we have to make sure we do not cross the short max. I'm glad at least one compiler caught this. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes@jabber.org VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL
Michael Meskes írta:
> On Sun, Jan 10, 2010 at 07:16:59PM +0100, Boszormenyi Zoltan wrote:
>
>> As would ecpg_dynamic_type(), then. :-(
>>
>
> My guess is that this function is fine when returning InvalidOid = 0. AFAICT it
> is supposed to fill an integer with the SQL3 type code which seems to start
> with 1 too. So I will change this one to return 0.
>
>
>> Perhaps InvalidOid wouldn't be the best choice to return,
>> because this function returns int, not Oid. InvalidOid equals
>> to ECPGt_char. Hm... it would be hiding the failure in
>>
>
> No, ECPGt_char is set to 1.
>
You're right.
>> a good way, as the type returned couldn't be mapped to
>> any ECPGt_* type, and the value would be returned in
>> a string anyway. We can use ECPGt_char for the unhandled case.
>>
>
> The question is, do we want to catch the unhandled case or shall we assume a
> string? Just tell me and I'll commit.
>
I think it's best to assume a string. ecpg_set_{compat,native}_sqlda()
uses the defailt case in that meaning anyway.
> Looking at the usage of sqlda_dynamic_type again we would run into this
> situation even earlier as the return value then is stort in a short int because
> that's what the other sqlda deffinitions use too. Therefore we have to make
> sure we do not cross the short max. I'm glad at least one compiler caught this.
>
> Michael
>
>
Thanks,
Zoltán Böszörményi
--
Bible has answers for everything. Proof:
"But let your communication be, Yea, yea; Nay, nay: for whatsoever is more
than these cometh of evil." (Matthew 5:37) - basics of digital technology.
"May your kingdom come" - superficial description of plate tectonics
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
http://www.postgresql.at/
On Wed, Jan 13, 2010 at 09:22:28AM +0100, Boszormenyi Zoltan wrote:
> I think it's best to assume a string. ecpg_set_{compat,native}_sqlda()
> uses the defailt case in that meaning anyway.
Okay, applied as you send it.
Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
ICQ: 179140304, AIM/Yahoo/Skype: michaelmeskes, Jabber: meskes@jabber.org
VfL Borussia! Forca Barca! Go SF 49ers! Use: Debian GNU/Linux, PostgreSQL