Re: Remove useless casting to the same type

Hi,

On Mon, Nov 24, 2025 at 03:18:00PM -0800, Jacob Champion wrote:
> On Mon, Nov 24, 2025 at 2:26 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Attached is a patch to $SUBJECT.
> 
> > --- a/src/backend/access/gin/gindatapage.c
> > +++ b/src/backend/access/gin/gindatapage.c
> > @@ -607,11 +607,11 @@ dataBeginPlaceToPageLeaf(GinBtree btree, Buffer buf, GinBtreeStack *stack,
> >
> >   if (append)
> >   elog(DEBUG2, "appended %d new items to block %u; %d bytes (%d to go)",
> > - maxitems, BufferGetBlockNumber(buf), (int) leaf->lsize,
> > + maxitems, BufferGetBlockNumber(buf), leaf->lsize,
> >   items->nitem - items->curitem - maxitems);
> 
> Hm. How do we feel, as a group, about superstitious casts in variadic
> calls? I don't feel like they're in the same class as the other fixes.
> 
> Argument for: it's nice to know at a glance that a printf() invocation
> won't corrupt something horribly, especially if I'm quickly scanning
> code during a CVE analysis, and especially if the variable is named as
> if it could maybe be a size_t. Do our compilers warn us well enough
> now, in practice?
> 
> Argument against: it takes up precious columns and focuses attention
> away from other things.

Thanks for looking at it!

I think that the variadic calls in the patch are related to functions that
can benefits from -Wformat. Let's focus on those: with the cast one would need
to verify 3 things: variable type, cast and format specifier.
Without the cast then only 2 things and the compiler can verify these match via
-Wformat warnings.

With the cast, the compiler only checks that the cast result matches the format,
not whether the cast itself is correct, so I'm in favor of removing the cast,
thoughts?

> Like the fact that (items->nitem -
> items->curitem - maxitems) is unsigned and printed as signed here. :D

Nice catch! ;-) 

> Maybe we should make the code compile cleanly under
> -Wformat-signedness at some point...

good idea, will give it a try later on outside the context of this patch.

> > @@ -389,7 +389,7 @@ hash_xlog_split_allocate_page(XLogReaderState *record)
> >
> >   /* extract low and high masks. */
> >   memcpy(&lowmask, data, sizeof(uint32));
> > - highmask = (uint32 *) ((char *) data + sizeof(uint32));
> > + highmask = (uint32 *) (data + sizeof(uint32));
> 
> I wonder about these, too. I like knowing what the code does without
> having to check the type of `data`. But then later on we do a `data +=
> sizeof(uint32) * 2`, so you have to check the type anyway, so... I
> don't know.

I think that even with the cast in place, it's good to check the type of data.
Not for the line that follows (i.e: "data += sizeof(uint32) * 2") but to check
that the cast makes sense and does not hide "wrong" pointer manipulation.

So I think that with or without the cast one would need to check. But that feels
more natural to check when there is no cast (as we don't assume that someone
said "I know what I'm doing"). So I'm in favor of removing the cast, thoughts?

> > +++ b/contrib/fuzzystrmatch/dmetaphone.c
> > @@ -327,7 +327,7 @@ GetAt(metastring *s, int pos)
> >   if ((pos < 0) || (pos >= s->length))
> >   return '\0';
> >
> > - return ((char) *(s->str + pos));
> > + return *(s->str + pos);
> 
> Isn't this just s->str[pos]? Ditto for SetAt(), right afterwards.

Yeah, "*(s->str + pos)" is already used in SetAt() and also in IsVowel(). Instead
of changing those 3, I'd prefer to keep the current change and keep the patch
focus on its intend. We could change those in a dedicated patch afterward if we
feel the need.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com