Horton

Thread

Commits

  1. Fix configure's AC_CHECK_DECLS tests to work correctly with clang.

  2. Suppress more variable-set-but-not-used warnings from clang 15.

  3. Suppress variable-set-but-not-used warnings from clang 15.

  1. Silencing the remaining clang 15 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-19T19:20:43Z

    While working on the -Wdeprecated-non-prototype fixups discussed
nearby, I saw that clang 15.0 produces a few other new warnings
(which are also visible in the buildfarm).  Pursuant to our
usual policy that we should suppress warnings on compilers likely
to be used for development, here's a patch to silence them.

There are three groups of these:

* With %pure-parser, Bison makes the "yynerrs" variable local
instead of static, and then if you don't use it clang notices
that it's set but never read.  There doesn't seem to be a way
to persuade Bison not to emit the variable at all, so here I've
just added "(void) yynerrs;" to the topmost production of each
affected grammar.  If anyone has a nicer idea, let's hear it.

* xlog.c's AdvanceXLInsertBuffer has a local variable "npages"
that is only read in the "#ifdef WAL_DEBUG" stanza at the
bottom.  Here I've done the rather ugly and brute-force thing
of wrapping all the variable's references in "#ifdef WAL_DEBUG".
(I tried marking it PG_USED_FOR_ASSERTS_ONLY, but oddly that
did not silence the warning.)  I kind of wonder how useful this
function's WAL_DEBUG output is --- maybe just dropping that
altogether would be better?

* array_typanalyze.c's compute_array_stats counts the number
of null arrays in the column, but then does nothing with the
result.  AFAICS this is redundant with what std_compute_stats
will do, so I just removed the variable.

Any thoughts?

			regards, tom lane

  2. Re: Silencing the remaining clang 15 warnings

    Thomas Munro <thomas.munro@gmail.com> — 2022-09-19T22:56:16Z

    On Tue, Sep 20, 2022 at 7:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * With %pure-parser, Bison makes the "yynerrs" variable local
> instead of static, and then if you don't use it clang notices
> that it's set but never read.  There doesn't seem to be a way
> to persuade Bison not to emit the variable at all, so here I've
> just added "(void) yynerrs;" to the topmost production of each
> affected grammar.  If anyone has a nicer idea, let's hear it.

+1.  FTR they know about this:

https://www.mail-archive.com/bison-patches@gnu.org/msg07836.html
https://github.com/akimd/bison/commit/a166d5450e3f47587b98f6005f9f5627dbe21a5b

... but that YY_ATTRIBUTE_UNUSED hasn't landed in my systems'
/usr[/local]/share/bison/skeletons/yacc.c yet and it seems harmless
and also legitimate to reference yynerrs from an action.

> * xlog.c's AdvanceXLInsertBuffer has a local variable "npages"
> that is only read in the "#ifdef WAL_DEBUG" stanza at the
> bottom.  Here I've done the rather ugly and brute-force thing
> of wrapping all the variable's references in "#ifdef WAL_DEBUG".
> (I tried marking it PG_USED_FOR_ASSERTS_ONLY, but oddly that
> did not silence the warning.)  I kind of wonder how useful this
> function's WAL_DEBUG output is --- maybe just dropping that
> altogether would be better?

No opinion on the value of the message, but maybe
pg_attribute_unused() would be better?

> * array_typanalyze.c's compute_array_stats counts the number
> of null arrays in the column, but then does nothing with the
> result.  AFAICS this is redundant with what std_compute_stats
> will do, so I just removed the variable.

+1

  3. Re: Silencing the remaining clang 15 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-19T23:06:53Z

    Thomas Munro <thomas.munro@gmail.com> writes:
> On Tue, Sep 20, 2022 at 7:20 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * xlog.c's AdvanceXLInsertBuffer has a local variable "npages"
>> that is only read in the "#ifdef WAL_DEBUG" stanza at the
>> bottom.  Here I've done the rather ugly and brute-force thing
>> of wrapping all the variable's references in "#ifdef WAL_DEBUG".
>> (I tried marking it PG_USED_FOR_ASSERTS_ONLY, but oddly that
>> did not silence the warning.)  I kind of wonder how useful this
>> function's WAL_DEBUG output is --- maybe just dropping that
>> altogether would be better?

> No opinion on the value of the message, but maybe
> pg_attribute_unused() would be better?

I realized that the reason PG_USED_FOR_ASSERTS_ONLY didn't help
is that it expands to empty in an assert-enabled build, which is
what I was testing.  So yeah, using pg_attribute_unused() directly
would probably work better.

(Also, I tried an assert-disabled build and found one additional new
warning; same deal where clang doesn't believe "foo++;" is reason to
consider foo to be used.  That one, PG_USED_FOR_ASSERTS_ONLY can fix.)

			regards, tom lane

  4. Re: Silencing the remaining clang 15 warnings

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-09-21T15:44:44Z

    HEAD and v15 now compile cleanly for me with clang 15.0.0,
but I find that there's still work to do in the back branches:

* There are new(?) -Wunused-but-set-variable warnings in every older
branch, which we evidently cleaned up or rewrote at one point or
another.  I think this is definitely worth fixing in the in-support
branches.  I'm a bit less sure if it's worth the trouble in the
out-of-support branches.

* The 9.2 and 9.3 branches spew boatloads of 'undeclared library function'
warnings about strlcpy() and related functions.  This is evidently
because 16fbac39f was only back-patched as far as 9.4.  There are
enough of these to be pretty annoying if you're trying to build those
branches with clang, so I think this is clearly justified for
back-patching into the older out-of-support branches, assuming that
the patch will work there.  (I see that 9.2 and 9.3 were still on the
prior version of autoconf, so it might not be an easy change.)

I also observe that 9.2 and 9.3 produce

float.c:1278:29: warning: implicit conversion from 'int' to 'float' changes value from 2147483647 to 2147483648 [-Wimplicit-const-int-float-conversion]

This is because cbdb8b4c0 was only back-patched as far as 9.4.
However, I think that that would *not* be fit material for
back-patching into out-of-support branches, since our policy
for them is "no behavioral changes".

			regards, tom lane