Remove useless casts to (void *)
Hi hackers,
I was working on something similar to [1]/messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org (which led to 7f798aca1d5) and I think
that 7f798aca1d5 missed removing some useless casts to (void *).
Meaning, the ones in the attached patch in:
- inval.c
- bump.c
- injection_point.c
- copyfromparse.c
- extension.c
- wparser.c
I did not find any reasons in the thread as to why those ones were not removed.
The attached also remove casts that have been added since 7f798aca1d5, the ones
in pg_publication.c, lock.c and tuplesortvariants.c.
The patch has been generated with the help of the .cocci script [2]https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/remove_useless_casts_to_void_star.cocci (though I
manually reviewed and removed some matches that, I think, were not appropriate).
[1]: /messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org
[2]: https://github.com/bdrouvot/coccinelle_on_pg/blob/main/misc/remove_useless_casts_to_void_star.cocci
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Remove-useless-casts-to-void.patchtext/x-diff; charset=us-asciiDownload
From 4ceb173dae4f1de9ef859d5d0dacb868427a53a0 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Thu, 20 Nov 2025 11:19:31 +0000
Subject: [PATCH v1] Remove useless casts to (void *)
Their presence causes (small) risks of hiding actual type mismatches or silently
discarding qualifiers. Some have been missed in 7f798aca1d5 and some are
new ones.
---
src/backend/catalog/pg_publication.c | 2 +-
src/backend/commands/copyfromparse.c | 2 +-
src/backend/commands/extension.c | 2 +-
src/backend/storage/lmgr/lock.c | 2 +-
src/backend/tsearch/wparser.c | 2 +-
src/backend/utils/cache/inval.c | 2 +-
src/backend/utils/misc/injection_point.c | 2 +-
src/backend/utils/mmgr/bump.c | 2 +-
src/backend/utils/sort/tuplesortvariants.c | 2 +-
9 files changed, 9 insertions(+), 9 deletions(-)
12.6% src/backend/catalog/
29.2% src/backend/commands/
12.3% src/backend/storage/lmgr/
10.0% src/backend/tsearch/
7.3% src/backend/utils/cache/
10.6% src/backend/utils/misc/
9.0% src/backend/utils/mmgr/
8.7% src/backend/utils/sort/
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index ac2f4ee3561..0994220c53d 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -1369,7 +1369,7 @@ pg_get_publication_sequences(PG_FUNCTION_ARGS)
if (publication->allsequences)
sequences = GetAllPublicationRelations(RELKIND_SEQUENCE, false);
- funcctx->user_fctx = (void *) sequences;
+ funcctx->user_fctx = sequences;
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index b1ae97b833d..a09e7fbace3 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -335,7 +335,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
if (avail > maxread)
avail = maxread;
pq_copymsgbytes(cstate->fe_msgbuf, databuf, avail);
- databuf = (void *) ((char *) databuf + avail);
+ databuf = (char *) databuf + avail;
maxread -= avail;
bytesread += avail;
}
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..ebc204c4462 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -931,7 +931,7 @@ execute_sql_string(const char *sql, const char *filename)
callback_arg.stmt_len = -1;
scripterrcontext.callback = script_error_callback;
- scripterrcontext.arg = (void *) &callback_arg;
+ scripterrcontext.arg = &callback_arg;
scripterrcontext.previous = error_context_stack;
error_context_stack = &scripterrcontext;
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 4cc7f645c31..9cb78ead105 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -1943,7 +1943,7 @@ WaitOnLock(LOCALLOCK *locallock, ResourceOwner owner)
/* Setup error traceback support for ereport() */
waiterrcontext.callback = waitonlock_error_callback;
- waiterrcontext.arg = (void *) locallock;
+ waiterrcontext.arg = locallock;
waiterrcontext.previous = error_context_stack;
error_context_stack = &waiterrcontext;
diff --git a/src/backend/tsearch/wparser.c b/src/backend/tsearch/wparser.c
index a8ddb610991..55171aa8da1 100644
--- a/src/backend/tsearch/wparser.c
+++ b/src/backend/tsearch/wparser.c
@@ -204,7 +204,7 @@ prs_setup_firstcall(FuncCallContext *funcctx, FunctionCallInfo fcinfo,
st->len = st->cur;
st->cur = 0;
- funcctx->user_fctx = (void *) st;
+ funcctx->user_fctx = st;
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
funcctx->tuple_desc = tupdesc;
diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
index 02505c88b8e..06f736cab45 100644
--- a/src/backend/utils/cache/inval.c
+++ b/src/backend/utils/cache/inval.c
@@ -1480,7 +1480,7 @@ CacheInvalidateHeapTupleCommon(Relation relation,
else
PrepareToInvalidateCacheTuple(relation, tuple, newtuple,
RegisterCatcacheInvalidation,
- (void *) info);
+ info);
/*
* Now, is this tuple one of the primary definers of a relcache entry? See
diff --git a/src/backend/utils/misc/injection_point.c b/src/backend/utils/misc/injection_point.c
index 64e6274652c..d02618c7ffe 100644
--- a/src/backend/utils/misc/injection_point.c
+++ b/src/backend/utils/misc/injection_point.c
@@ -186,7 +186,7 @@ injection_point_cache_load(InjectionPointEntry *entry, int slot_idx, uint64 gene
elog(ERROR, "could not find library \"%s\" for injection point \"%s\"",
path, entry->name);
- injection_callback_local = (void *)
+ injection_callback_local =
load_external_function(path, entry->function, false, NULL);
if (injection_callback_local == NULL)
diff --git a/src/backend/utils/mmgr/bump.c b/src/backend/utils/mmgr/bump.c
index a263861fe43..e60ec94e139 100644
--- a/src/backend/utils/mmgr/bump.c
+++ b/src/backend/utils/mmgr/bump.c
@@ -407,7 +407,7 @@ BumpAllocChunkFromBlock(MemoryContext context, BumpBlock *block, Size size,
#ifdef MEMORY_CONTEXT_CHECKING
chunk = (MemoryChunk *) block->freeptr;
#else
- ptr = (void *) block->freeptr;
+ ptr = block->freeptr;
#endif
/* point the freeptr beyond this chunk */
diff --git a/src/backend/utils/sort/tuplesortvariants.c b/src/backend/utils/sort/tuplesortvariants.c
index 41ac4afbf49..9751a7fc495 100644
--- a/src/backend/utils/sort/tuplesortvariants.c
+++ b/src/backend/utils/sort/tuplesortvariants.c
@@ -1954,7 +1954,7 @@ readtup_index_gin(Tuplesortstate *state, SortTuple *stup,
LogicalTapeReadExact(tape, tuple, tuplen);
if (base->sortopt & TUPLESORT_RANDOMACCESS) /* need trailing length word? */
LogicalTapeReadExact(tape, &tuplen, sizeof(tuplen));
- stup->tuple = (void *) tuple;
+ stup->tuple = tuple;
/* no abbreviations (FIXME maybe use attrnum for this?) */
stup->datum1 = (Datum) 0;
--
2.34.1
Hi Bertrand,
The attached also remove casts that have been added since 7f798aca1d5, the ones
in pg_publication.c, lock.c and tuplesortvariants.c.The patch has been generated with the help of the .cocci script [2] (though I
manually reviewed and removed some matches that, I think, were not appropriate).
I didn't review the entire patch but one change caught my attention:
```
- databuf = (void *) ((char *) databuf + avail);
+ databuf = (char *) databuf + avail;
```
Here `databuf` has a type (void*). Although the code is correct, it
replaces an explicit cast (which I read "yes, we know what we are
doing") with an implicit one. Personally I don't think this is a good
change.
These were just my two cents. All in all I'm neither for nor against the patch.
--
Best regards,
Aleksander Alekseev
Hi Aleksander,
On Thu, Nov 20, 2025 at 05:01:49PM +0300, Aleksander Alekseev wrote:
Hi Bertrand,
The attached also remove casts that have been added since 7f798aca1d5, the ones
in pg_publication.c, lock.c and tuplesortvariants.c.The patch has been generated with the help of the .cocci script [2] (though I
manually reviewed and removed some matches that, I think, were not appropriate).I didn't review the entire patch but one change caught my attention:
Thanks for looking at it!
``` - databuf = (void *) ((char *) databuf + avail); + databuf = (char *) databuf + avail; ```Here `databuf` has a type (void*). Although the code is correct, it
replaces an explicit cast (which I read "yes, we know what we are
doing") with an implicit one.
Yes, that's what it is doing and so was 7f798aca1d5.
For example in 7f798aca1d5, you can also see things like:
@@ -858,7 +858,7 @@ setup_firstcall(FuncCallContext *funcctx, HStore *hs,
st = (HStore *) palloc(VARSIZE(hs));
memcpy(st, hs, VARSIZE(hs));
- funcctx->user_fctx = (void *) st;
+ funcctx->user_fctx = st;
Where funcctx->user_fctx is of type (void *) and st is of type (HStore *).
Personally I don't think this is a good change.
Only this one (because maybe databuf is used twice) or the whole idea of 7f798aca1d5
and this patch?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Hi,
Personally I don't think this is a good change.
Only this one (because maybe databuf is used twice) or the whole idea of 7f798aca1d5
and this patch?
The whole idea to be honest. This being said, this is just my personal
opinion as of today. The majority of the community may disagree and/or
have good arguments to do this I'm not aware of or failed to
understand and/or just have other sense of beauty. And that's OK. IMO
what actually is important is for the code to be consistent. That's as
I understand what your patch is trying to accomplish.
Let's see what other people think.
--
Best regards,
Aleksander Alekseev
On 2025-Nov-20, Aleksander Alekseev wrote:
IMO what actually is important is for the code to be consistent.
IMO what is most important is that the code is correct. Second most
important is that the code is performant. The consistency is perhaps a
third priority, if that -- there may be other objectives that are also
more important than consistency.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Hi,
On Thu, Nov 20, 2025 at 04:43:58PM +0100, �lvaro Herrera wrote:
On 2025-Nov-20, Aleksander Alekseev wrote:
IMO what actually is important is for the code to be consistent.
IMO what is most important is that the code is correct. Second most
important is that the code is performant. The consistency is perhaps a
third priority, if that -- there may be other objectives that are also
more important than consistency.
Yeah, I was convinced by the discussion in [1]/messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org, so spent some time to be able
to detect kind of automatically those useless casts.
[1]: /messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Thu, Nov 20, 2025 at 6:02 AM Aleksander Alekseev
<aleksander@tigerdata.com> wrote:
Here `databuf` has a type (void*). Although the code is correct, it
replaces an explicit cast (which I read "yes, we know what we are
doing") with an implicit one.
"Yes, we know what we are doing" is the argument against doing it,
though. There's no upside to telling the compiler that in this case:
if it's correct, the compiler would have done it for you anyway, and
if it's buggy, now the compiler has been told to stay silent.
So +1 on removing unneeded (void *) casts in general, for the sake of
establishing consensus, though I haven't looked at this particular
patch in detail.
--Jacob