Обсуждение: Silencing the remaining clang 15 warnings
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
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 81d339d57d..2651bcaa76 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1785,7 +1785,9 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
XLogRecPtr NewPageEndPtr = InvalidXLogRecPtr;
XLogRecPtr NewPageBeginPtr;
XLogPageHeader NewPage;
+#ifdef WAL_DEBUG
int npages = 0;
+#endif
LWLockAcquire(WALBufMappingLock, LW_EXCLUSIVE);
@@ -1928,7 +1930,9 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
XLogCtl->InitializedUpTo = NewPageEndPtr;
+#ifdef WAL_DEBUG
npages++;
+#endif
}
LWLockRelease(WALBufMappingLock);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 82f03fc9c9..c6ec694ceb 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -864,6 +864,7 @@ parse_toplevel:
stmtmulti
{
pg_yyget_extra(yyscanner)->parsetree = $1;
+ (void) yynerrs; /* suppress compiler warning */
}
| MODE_TYPE_NAME Typename
{
diff --git a/src/backend/utils/adt/array_typanalyze.c b/src/backend/utils/adt/array_typanalyze.c
index 1964cedd93..2360c680ac 100644
--- a/src/backend/utils/adt/array_typanalyze.c
+++ b/src/backend/utils/adt/array_typanalyze.c
@@ -218,7 +218,6 @@ compute_array_stats(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfunc,
{
ArrayAnalyzeExtraData *extra_data;
int num_mcelem;
- int null_cnt = 0;
int null_elem_cnt = 0;
int analyzed_rows = 0;
@@ -320,8 +319,7 @@ compute_array_stats(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfunc,
value = fetchfunc(stats, array_no, &isnull);
if (isnull)
{
- /* array is null, just count that */
- null_cnt++;
+ /* ignore arrays that are null overall */
continue;
}
diff --git a/src/backend/utils/adt/jsonpath_gram.y b/src/backend/utils/adt/jsonpath_gram.y
index 7e28853a57..2a56629cc3 100644
--- a/src/backend/utils/adt/jsonpath_gram.y
+++ b/src/backend/utils/adt/jsonpath_gram.y
@@ -113,6 +113,7 @@ result:
*result = palloc(sizeof(JsonPathParseResult));
(*result)->expr = $2;
(*result)->lax = $1;
+ (void) yynerrs;
}
| /* EMPTY */ { *result = NULL; }
;
diff --git a/src/bin/pgbench/exprparse.y b/src/bin/pgbench/exprparse.y
index ade2ecdaab..f6387d4a3c 100644
--- a/src/bin/pgbench/exprparse.y
+++ b/src/bin/pgbench/exprparse.y
@@ -80,7 +80,10 @@ static PgBenchExpr *make_case(yyscan_t yyscanner, PgBenchExprList *when_then_lis
%%
-result: expr { expr_parse_result = $1; }
+result: expr {
+ expr_parse_result = $1;
+ (void) yynerrs; /* suppress compiler warning */
+ }
elist: { $$ = NULL; }
| expr { $$ = make_elist($1, NULL); }
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
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
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