Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
Chapman Flackwrites: > It might be fun to see how big a chunk of the 4106 would vanish just > with the first tweak to one of the causes that's mentioned in a lot of > them. (Unless your figures were already after culling to distinct causes, > which would sound like a more-than-casual effort.) No, I just did "make 2>&1 | grep 'warning: conversion' | wc". I did look through the warnings a little bit. A lot of them seem to be caused by our being cavalier about using "int" parameters and/or loop variables to represent attribute numbers; as soon as you pass one of those to an API that's declared AttrNumber, warning. Another large batch are from conversions from size_t to int, a practice that's perfectly safe for palloc'd values. And I saw some in the planner from conversions of rowcounts to double --- yes, I know that's imprecise, thank you very much. Based on what I saw, there are hardly any places where a single touch would remove a large number of these warnings; it'd be more like fixing them retail, and it would be pretty pointless. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
On 06/01/17 17:41, Tom Lane wrote: > 12169 warnings generated by -Wconversion > 4106 warnings generated by -Wconversion -Wno-sign-conversion > ... > So it's better with -Wno-sign-conversion, but I'd say we're still not > going there anytime soon. On an optimistic note, there might not turn out to be anywhere near as many distinct causes; there's typically a lot of amplification. The one patch I sent in eliminated screens upon screens of warning output from the PL/Java build (I made no effort to count them, I just listened to the noise in my speakers until I heard the scrolling stop). It might be fun to see how big a chunk of the 4106 would vanish just with the first tweak to one of the causes that's mentioned in a lot of them. (Unless your figures were already after culling to distinct causes, which would sound like a more-than-casual effort.) Trouble is, after that first taste of success beyond expectation, it gets like a drug. -Chap -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
Chapman Flackwrites: > On 05/31/2017 11:36 AM, Tom Lane wrote: >> However, I grant your point that some extensions may have outside >> constraints that mandate using -Wconversion, so to the extent that >> we can keep key headers like postgres.h from triggering those warnings, >> it's probably worth doing. I suspect you're still seeing a lot of them >> though --- experiments with some contrib modules suggest that a lot of >> our other headers also contain code that would trigger them. I do not >> think I'd be on board with trying to silence them generally. > That was actually the only one PL/Java gets, outside of /sign/ > conversions, a special subset of conversion warnings that can be > separately turned off with -Wno-sign-conversion. Just for the archives' sake: I experimented with this, using Fedora 25's compiler (gcc version 6.3.1) against current HEAD (including your patch). For the core build only, no contrib, I see: 12169 warnings generated by -Wconversion 4106 warnings generated by -Wconversion -Wno-sign-conversion It's not just the core code that has issues either: contrib has 2202 warnings for the first case, 683 for the second. So it's better with -Wno-sign-conversion, but I'd say we're still not going there anytime soon. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
On 05/31/2017 11:36 AM, Tom Lane wrote: > However, I grant your point that some extensions may have outside > constraints that mandate using -Wconversion, so to the extent that > we can keep key headers like postgres.h from triggering those warnings, > it's probably worth doing. I suspect you're still seeing a lot of them > though --- experiments with some contrib modules suggest that a lot of > our other headers also contain code that would trigger them. I do not > think I'd be on board with trying to silence them generally. That was actually the only one PL/Java gets, outside of /sign/ conversions, a special subset of conversion warnings that can be separately turned off with -Wno-sign-conversion. There are gobs of those, which I looked into briefly last year, realized they'd only be fixed by a bunch of cluttery unenlightening casts in pg headers, which I did not propose, having a suspicion that's how you would feel about them. I added a Maven build profile that adds the -Wno-sign-conversion (though actually getting Maven to select that profile automatically when the compiler is gcc is one of those things you'd expect to be easy and then be surprised.) But as long as there used to be no extraneous noise outside of /sign/ conversions, I figured this one new appearance of a warning outside that category would be worth silencing, before some day comes when there are three and it seems like an organization, or fifty and it seems like a movement. -Chap -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
Chapman Flackwrites: > On 05/31/17 01:26, Tom Lane wrote: >> Hm. I think it would be better to use DatumGetInt32 here. Arguably, >> direct use of GET_4_BYTES and its siblings should only appear in >> DatumGetFoo macros. > Like so? These are the 4 sites where {GET,SET}_n_BYTES got introduced > in 14cca1b (for consistency, though only the GET_4 case produces warnings). After experimenting with -Wconversion, I see why we don't use it in server builds --- it produces an astonishing number of mostly-useless warnings, which apparently can only be silenced by introducing explicit casts. I do not think that cluttering our code with lots more explicit casts would be a win for either readability or safety. However, I grant your point that some extensions may have outside constraints that mandate using -Wconversion, so to the extent that we can keep key headers like postgres.h from triggering those warnings, it's probably worth doing. I suspect you're still seeing a lot of them though --- experiments with some contrib modules suggest that a lot of our other headers also contain code that would trigger them. I do not think I'd be on board with trying to silence them generally. However, the present patch seems harmless enough, and arguably a tad cleaner than what we had, so pushed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
Chapman Flackwrites: > On 05/31/17 01:26, Tom Lane wrote: >> Hm. I think it would be better to use DatumGetInt32 here. Arguably, >> direct use of GET_4_BYTES and its siblings should only appear in >> DatumGetFoo macros. > Like so? Looks promising offhand, but I'm too tired to check in detail right now. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
On 05/31/17 01:26, Tom Lane wrote: > Hm. I think it would be better to use DatumGetInt32 here. Arguably, > direct use of GET_4_BYTES and its siblings should only appear in > DatumGetFoo macros. Like so? These are the 4 sites where {GET,SET}_n_BYTES got introduced in 14cca1b (for consistency, though only the GET_4 case produces warnings). -Chap --- src/include/postgres.h 2017-05-31 01:36:16.621829183 -0400 +++ src/include/postgres.h 2017-05-31 01:45:51.303427115 -0400 @@ -679,7 +679,7 @@ float4 retval; } myunion; - myunion.value = GET_4_BYTES(X); + myunion.value = DatumGetInt32(X); return myunion.retval; } #else @@ -704,7 +704,7 @@ } myunion; myunion.value = X; - return SET_4_BYTES(myunion.retval); + return Int32GetDatum(myunion.retval); } #else extern Datum Float4GetDatum(float4 X); @@ -727,7 +727,7 @@ float8 retval; } myunion; - myunion.value = GET_8_BYTES(X); + myunion.value = DatumGetInt64(X); return myunion.retval; } #else @@ -753,7 +753,7 @@ } myunion; myunion.value = X; - return SET_8_BYTES(myunion.retval); + return Int64GetDatum(myunion.retval); } #else extern Datum Float8GetDatum(float8 X); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
Chapman Flackwrites: > - myunion.value = GET_4_BYTES(X); > + myunion.value = (int32)GET_4_BYTES(X); Hm. I think it would be better to use DatumGetInt32 here. Arguably, direct use of GET_4_BYTES and its siblings should only appear in DatumGetFoo macros. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] quiet conversion warning in DatumGetFloat4
It seems that 14cca1b (use static inline functions for float <-> Datum conversions) has an implicit narrowing conversion in one of those functions. If building an extension with gcc's -Wconversion warning enabled (*cough* pljava *cough* ... the Maven plugin that runs the compiler enables the warning by default), this makes for a noisy build. The warning is harmless, but repeated everywhere postgres.h is included. An explicit cast is enough to suppress it. -Chap --- src/include/postgres.h 2017-05-15 17:20:59.0 -0400 +++ src/include/postgres.h 2017-05-31 00:21:27.329393499 -0400 @@ -679,7 +679,7 @@ float4 retval; } myunion; - myunion.value = GET_4_BYTES(X); + myunion.value = (int32)GET_4_BYTES(X); return myunion.retval; } #else -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers