pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedclang-tidy complaints
7+ messages / 4 participants
[nested] [flat]
* clang-tidy complaints
@ 2025-04-12 16:25 Peter Geoghegan <pg@bowt.ie>
0 siblings, 2 replies; 7+ messages in thread
From: Peter Geoghegan @ 2025-04-12 16:25 UTC (permalink / raw)
To: pgsql-committers <pgsql-committers@lists.postgresql.org>
I just pushed my annual commit that makes function parameter names
consistent between function declarations and their corresponding
definitions, to cover the Postgres 18 cycle. The details there were
all straightforward. But there are 2 remaining complaints from
clang-tidy that seem worth fixing, that don't quite fall under any
established policy in this area.
The first such complaint concerns a new mcxt.c function parameter that
shadows a global variable in the same file -- attached patch fixes
that by renaming the function parameter. Technically, this is a
distinct type of complaint to the clang-tidy complaints that I
ordinarily fix this time of year, though it's of the same general
nature.
The second such complaint is a standard "function parameter name is
inconsistent" complaint, though it's one that affects vendored code in
src/include/snowball/libstemmer/header.h. I attach a fix for that,
too. In the past we have tended to treat cases with vendored code just
like non-vendored Postgres code, but there's always a question about
whether it's worth diverging from upstream. That needs to be decided
on a case-by-case basis, and I don't know enough to know if I should
proceed here.
I have not attached a fix for a similar vendored code issue that
appears in timingsafe_bcmp.c, since that involved a declaration
provided by my system's openSSL being inconsistent with our own
vendored definition. A fix for that seemed unnecessary.
--
Peter Geoghegan
Attachments: download all (2 files, .tar.gz)
v1-0002-Make-libstemmer-param-name-consistent.patch (application/octet-stream, 1.2K) download
From 95aa03cbe865be442dd540b620299ee410e5a61a Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 12 Apr 2025 11:48:10 -0400
Subject: [PATCH v1 2/2] Make libstemmer param name consistent.
---
src/include/snowball/libstemmer/header.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/snowball/libstemmer/header.h b/src/include/snowball/libstemmer/header.h
index bf172d5b9..ef5a54640 100644
--- a/src/include/snowball/libstemmer/header.h
+++ b/src/include/snowball/libstemmer/header.h
@@ -45,7 +45,7 @@ extern int eq_v_b(struct SN_env * z, const symbol * p);
extern int find_among(struct SN_env * z, const struct among * v, int v_size);
extern int find_among_b(struct SN_env * z, const struct among * v, int v_size);
-extern int replace_s(struct SN_env * z, int c_bra, int c_ket, int s_size, const symbol * s, int * adjustment);
+extern int replace_s(struct SN_env * z, int c_bra, int c_ket, int s_size, const symbol * s, int * adjptr);
extern int slice_from_s(struct SN_env * z, int s_size, const symbol * s);
extern int slice_from_v(struct SN_env * z, const symbol * p);
extern int slice_del(struct SN_env * z);
--
2.49.0
v1-0001-Stop-shadowing-global-variable-with-parameter-nam.patch (application/octet-stream, 4.3K) download
From 3ff696475b17157ccb949236286d19b27fafc19e Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 12 Apr 2025 11:47:35 -0400
Subject: [PATCH v1 1/2] Stop shadowing global variable with parameter name in
mcxt.c.
---
src/backend/utils/mmgr/mcxt.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 1f5ebf2e1..fa495cbfd 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -188,13 +188,13 @@ static void PublishMemoryContext(MemoryStatsEntry *memcxt_info,
int curr_id, MemoryContext context,
List *path,
MemoryContextCounters stat,
- int num_contexts, dsa_area *area,
+ int num_contexts, dsa_area *darea,
int max_levels);
static void compute_contexts_count_and_ids(List *contexts, HTAB *context_id_lookup,
int *stats_count,
bool summary);
static List *compute_context_path(MemoryContext c, HTAB *context_id_lookup);
-static void free_memorycontextstate_dsa(dsa_area *area, int total_stats,
+static void free_memorycontextstate_dsa(dsa_area *darea, int total_stats,
dsa_pointer prev_dsa_pointer);
static void end_memorycontext_reporting(void);
@@ -1790,7 +1790,7 @@ static void
PublishMemoryContext(MemoryStatsEntry *memcxt_info, int curr_id,
MemoryContext context, List *path,
MemoryContextCounters stat, int num_contexts,
- dsa_area *area, int max_levels)
+ dsa_area *darea, int max_levels)
{
const char *ident = context->ident;
const char *name = context->name;
@@ -1815,8 +1815,8 @@ PublishMemoryContext(MemoryStatsEntry *memcxt_info, int curr_id,
namelen = pg_mbcliplen(name, namelen,
MEMORY_CONTEXT_IDENT_SHMEM_SIZE - 1);
- memcxt_info[curr_id].name = dsa_allocate(area, namelen + 1);
- nameptr = (char *) dsa_get_address(area, memcxt_info[curr_id].name);
+ memcxt_info[curr_id].name = dsa_allocate(darea, namelen + 1);
+ nameptr = (char *) dsa_get_address(darea, memcxt_info[curr_id].name);
strlcpy(nameptr, name, namelen + 1);
}
else
@@ -1836,8 +1836,8 @@ PublishMemoryContext(MemoryStatsEntry *memcxt_info, int curr_id,
idlen = pg_mbcliplen(ident, idlen,
MEMORY_CONTEXT_IDENT_SHMEM_SIZE - 1);
- memcxt_info[curr_id].ident = dsa_allocate(area, idlen + 1);
- identptr = (char *) dsa_get_address(area, memcxt_info[curr_id].ident);
+ memcxt_info[curr_id].ident = dsa_allocate(darea, idlen + 1);
+ identptr = (char *) dsa_get_address(darea, memcxt_info[curr_id].ident);
strlcpy(identptr, ident, idlen + 1);
}
else
@@ -1851,9 +1851,9 @@ PublishMemoryContext(MemoryStatsEntry *memcxt_info, int curr_id,
int levels = Min(list_length(path), max_levels);
memcxt_info[curr_id].path_length = levels;
- memcxt_info[curr_id].path = dsa_allocate0(area, levels * sizeof(int));
+ memcxt_info[curr_id].path = dsa_allocate0(darea, levels * sizeof(int));
memcxt_info[curr_id].levels = list_length(path);
- path_list = (int *) dsa_get_address(area, memcxt_info[curr_id].path);
+ path_list = (int *) dsa_get_address(darea, memcxt_info[curr_id].path);
foreach_int(i, path)
{
@@ -1877,26 +1877,26 @@ PublishMemoryContext(MemoryStatsEntry *memcxt_info, int curr_id,
* responsible for ensuring that the DSA pointer is valid.
*/
static void
-free_memorycontextstate_dsa(dsa_area *area, int total_stats,
+free_memorycontextstate_dsa(dsa_area *darea, int total_stats,
dsa_pointer prev_dsa_pointer)
{
MemoryStatsEntry *meminfo;
- meminfo = (MemoryStatsEntry *) dsa_get_address(area, prev_dsa_pointer);
+ meminfo = (MemoryStatsEntry *) dsa_get_address(darea, prev_dsa_pointer);
Assert(meminfo != NULL);
for (int i = 0; i < total_stats; i++)
{
if (DsaPointerIsValid(meminfo[i].name))
- dsa_free(area, meminfo[i].name);
+ dsa_free(darea, meminfo[i].name);
if (DsaPointerIsValid(meminfo[i].ident))
- dsa_free(area, meminfo[i].ident);
+ dsa_free(darea, meminfo[i].ident);
if (DsaPointerIsValid(meminfo[i].path))
- dsa_free(area, meminfo[i].path);
+ dsa_free(darea, meminfo[i].path);
}
- dsa_free(area, memCxtState[MyProcNumber].memstats_dsa_pointer);
+ dsa_free(darea, memCxtState[MyProcNumber].memstats_dsa_pointer);
memCxtState[MyProcNumber].memstats_dsa_pointer = InvalidDsaPointer;
}
--
2.49.0
* Re: clang-tidy complaints
@ 2025-04-12 16:35 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Geoghegan <pg@bowt.ie>
1 sibling, 1 reply; 7+ messages in thread
From: Tom Lane @ 2025-04-12 16:35 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
Peter Geoghegan <pg@bowt.ie> writes:
> The first such complaint concerns a new mcxt.c function parameter that
> shadows a global variable in the same file -- attached patch fixes
> that by renaming the function parameter. Technically, this is a
> distinct type of complaint to the clang-tidy complaints that I
> ordinarily fix this time of year, though it's of the same general
> nature.
Isn't that obsolete in the wake of 55ef7abf8? I'd be inclined to
leave it alone if no longer needed, because (to me anyway) "darea"
reads worse than "area".
> The second such complaint is a standard "function parameter name is
> inconsistent" complaint, though it's one that affects vendored code in
> src/include/snowball/libstemmer/header.h. I attach a fix for that,
> too. In the past we have tended to treat cases with vendored code just
> like non-vendored Postgres code, but there's always a question about
> whether it's worth diverging from upstream. That needs to be decided
> on a case-by-case basis, and I don't know enough to know if I should
> proceed here.
I don't want to diverge from upstream snowball here, because we do
absorb new snowball versions every so often, so the problem would just
come back. Maybe you could write to upstream and see if they'd accept
the change? If they do, it'd be fine to apply locally.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: clang-tidy complaints
@ 2025-04-12 16:47 Peter Geoghegan <pg@bowt.ie>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Peter Geoghegan @ 2025-04-12 16:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sat, Apr 12, 2025 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
> > The first such complaint concerns a new mcxt.c function parameter that
> > shadows a global variable in the same file -- attached patch fixes
> > that by renaming the function parameter. Technically, this is a
> > distinct type of complaint to the clang-tidy complaints that I
> > ordinarily fix this time of year, though it's of the same general
> > nature.
>
> Isn't that obsolete in the wake of 55ef7abf8? I'd be inclined to
> leave it alone if no longer needed, because (to me anyway) "darea"
> reads worse than "area".
I missed that, because I was working against a branch that diverged
with HEAD as of a couple of days ago. You're right -- it's now
obsolete.
> I don't want to diverge from upstream snowball here, because we do
> absorb new snowball versions every so often, so the problem would just
> come back. Maybe you could write to upstream and see if they'd accept
> the change? If they do, it'd be fine to apply locally.
Okay. I don't think that it's worth pursuing upstream. In general I
don't expect clang-tidy to have zero "inconsistent parameter" names at
any time, due to a number of special cases (mostly with
machine-generated code that uses flex).
Looks like I'm done with this process, at least until this time next year.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: clang-tidy complaints
@ 2025-04-12 17:16 Tom Lane <tgl@sss.pgh.pa.us>
parent: Peter Geoghegan <pg@bowt.ie>
0 siblings, 1 reply; 7+ messages in thread
From: Tom Lane @ 2025-04-12 17:16 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
Peter Geoghegan <pg@bowt.ie> writes:
> Looks like I'm done with this process, at least until this time next year.
OK. Thanks for taking care of it.
regards, tom lane
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: clang-tidy complaints
@ 2025-04-26 20:19 John Naylor <johncnaylorls@gmail.com>
parent: Peter Geoghegan <pg@bowt.ie>
1 sibling, 0 replies; 7+ messages in thread
From: John Naylor @ 2025-04-26 20:19 UTC (permalink / raw)
To: Peter Geoghegan <pg@bowt.ie>; +Cc: pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sat, Apr 12, 2025 at 11:25 PM Peter Geoghegan <pg@bowt.ie> wrote:
>
> I just pushed my annual commit that makes function parameter names
> consistent between function declarations and their corresponding
> definitions, to cover the Postgres 18 cycle.
The new CRC function this touched was right in the header but the
parameter in the definition didn't match match surrounding code (my
fault in 3c6e8c123), so I went ahead and fixed up the definition. (As
an aside, one wrinkle in the clang-tidy result is that it only touched
the declaration in the "runtime check" stanza and not the "targeting
SSE 4.2" stanza.)
--
John Naylor
Amazon Web Services
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: clang-tidy complaints
@ 2025-04-27 08:25 Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 7+ messages in thread
From: Bertrand Drouvot @ 2025-04-27 08:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Peter Geoghegan <pg@bowt.ie>; pgsql-committers <pgsql-committers@lists.postgresql.org>
Hi,
On Sat, Apr 12, 2025 at 01:16:39PM -0400, Tom Lane wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
> > Looks like I'm done with this process, at least until this time next year.
>
> OK. Thanks for taking care of it.
+1. Peter, do you think you could add "misc-header-include-cycle" in your
annual check (see [1])? I'll also put a note on my side to do it at a regular
basis.
[1]: https://www.postgresql.org/message-id/2238517.1745644856%40sss.pgh.pa.us
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: clang-tidy complaints
@ 2025-04-27 15:50 Peter Geoghegan <pg@bowt.ie>
parent: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
0 siblings, 0 replies; 7+ messages in thread
From: Peter Geoghegan @ 2025-04-27 15:50 UTC (permalink / raw)
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-committers <pgsql-committers@lists.postgresql.org>
On Sun, Apr 27, 2025 at 4:25 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> +1. Peter, do you think you could add "misc-header-include-cycle" in your
> annual check (see [1])? I'll also put a note on my side to do it at a regular
> basis.
I've added it to my personal clangd config (I mostly use clang-tidy
through clangd). I'll try to remember to use it when I run clang-tidy
a year from now, to clean up the master branch at the end of the
Postgres 19 cycle.
--
Peter Geoghegan
^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2025-04-27 15:50 UTC | newest]
Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-12 16:25 clang-tidy complaints Peter Geoghegan <pg@bowt.ie>
2025-04-12 16:35 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-04-12 16:47 ` Peter Geoghegan <pg@bowt.ie>
2025-04-12 17:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-04-27 08:25 ` Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
2025-04-27 15:50 ` Peter Geoghegan <pg@bowt.ie>
2025-04-26 20:19 ` John Naylor <johncnaylorls@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox