Horton

Thread

Commits

  1. Avoid -Wconversion warnings from direct use of GET_n_BYTES macros.

  1. [PATCH] quiet conversion warning in DatumGetFloat4

    Chapman Flack <chap@anastigmatix.net> — 2017-05-31T04:56:36Z

    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

  2. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-31T05:26:30Z

    Chapman Flack <chap@anastigmatix.net> writes:
> -	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

  3. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Chapman Flack <chap@anastigmatix.net> — 2017-05-31T05:55:11Z

    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

  4. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-31T05:57:09Z

    Chapman Flack <chap@anastigmatix.net> writes:
> 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

  5. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-05-31T15:36:18Z

    Chapman Flack <chap@anastigmatix.net> writes:
> 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

  6. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Chapman Flack <chap@anastigmatix.net> — 2017-05-31T17:51:20Z

    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

  7. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-01T21:41:25Z

    Chapman Flack <chap@anastigmatix.net> writes:
> 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

  8. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Chapman Flack <chap@anastigmatix.net> — 2017-06-01T22:05:27Z

    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

  9. Re: [PATCH] quiet conversion warning in DatumGetFloat4

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-01T22:20:50Z

    Chapman Flack <chap@anastigmatix.net> writes:
> 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