Обсуждение: Remove useless casts to (void *)
Hi hackers, I was working on something similar to [1] (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] (though I manually reviewed and removed some matches that, I think, were not appropriate). [1]: https://www.postgresql.org/message-id/flat/461ea37c-8b58-43b4-9736-52884e862820%40eisentraut.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
Вложения
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], so spent some time to be able to detect kind of automatically those useless casts. [1]: https://www.postgresql.org/message-id/flat/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
On 20.11.25 14:33, Bertrand Drouvot wrote: > I was working on something similar to [1] (which led to 7f798aca1d5) and I think > that 7f798aca1d5 missed removing some useless casts to (void *). committed, thanks