Thread: Re: Remove useless casts to (void *)

Re: Remove useless casts to (void *)

From
Tom Lane
Date:
Peter Eisentraut <peter@eisentraut.org> writes:
> There are a bunch of (void *) casts in the code that don't make sense to 
> me.  I think some of these were once necessary because char * was used 
> in place of void * for some function arguments.  And some of these were 
> probably just copied around without further thought.  I went through and 
> cleaned up most of these.  I didn't find any redeeming value in these. 
> They are just liable to hide actual problems such as incompatible types. 
>   But maybe there are other opinions.

I don't recall details, but I'm fairly sure some of these prevented
compiler warnings on some (old?) compilers.  Hard to be sure if said
compilers are all gone.

Looking at the sheer size of the patch, I'm kind of -0.1, just
because I'm afraid it's going to create back-patching gotchas.
I don't really find that it's improving readability, though
clearly that's a matter of opinion.

            regards, tom lane



Re: Remove useless casts to (void *)

From
Bruce Momjian
Date:
On Tue, Oct 29, 2024 at 10:20:03AM -0400, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
> > There are a bunch of (void *) casts in the code that don't make sense to 
> > me.  I think some of these were once necessary because char * was used 
> > in place of void * for some function arguments.  And some of these were 
> > probably just copied around without further thought.  I went through and 
> > cleaned up most of these.  I didn't find any redeeming value in these. 
> > They are just liable to hide actual problems such as incompatible types. 
> >   But maybe there are other opinions.
> 
> I don't recall details, but I'm fairly sure some of these prevented
> compiler warnings on some (old?) compilers.  Hard to be sure if said
> compilers are all gone.
> 
> Looking at the sheer size of the patch, I'm kind of -0.1, just
> because I'm afraid it's going to create back-patching gotchas.
> I don't really find that it's improving readability, though
> clearly that's a matter of opinion.

I kind of liked the patch in terms of simplifying things.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"



Re: Remove useless casts to (void *)

From
Peter Eisentraut
Date:
On 29.10.24 15:20, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> There are a bunch of (void *) casts in the code that don't make sense to
>> me.  I think some of these were once necessary because char * was used
>> in place of void * for some function arguments.  And some of these were
>> probably just copied around without further thought.  I went through and
>> cleaned up most of these.  I didn't find any redeeming value in these.
>> They are just liable to hide actual problems such as incompatible types.
>>    But maybe there are other opinions.
> 
> I don't recall details, but I'm fairly sure some of these prevented
> compiler warnings on some (old?) compilers.  Hard to be sure if said
> compilers are all gone.
> 
> Looking at the sheer size of the patch, I'm kind of -0.1, just
> because I'm afraid it's going to create back-patching gotchas.
> I don't really find that it's improving readability, though
> clearly that's a matter of opinion.

I did a bit of archeological research on these.  None of these casts 
were ever necessary, and in many cases even the original patch that 
introduced an API used the coding style inconsistently.  So I'm very 
confident that there are no significant backward compatibility or 
backpatching gotchas here.

I'm more concerned that many of these just keep getting copied around 
indiscriminately, and this is liable to hide actual type mismatches or 
silently discard qualifiers.  So I'm arguing in favor of a more 
restrictive style in this matter.




Re: Remove useless casts to (void *)

From
Bruce Momjian
Date:
On Thu, Nov 14, 2024 at 09:59:07AM +0100, Peter Eisentraut wrote:
> On 29.10.24 15:20, Tom Lane wrote:
> > Peter Eisentraut <peter@eisentraut.org> writes:
> > > There are a bunch of (void *) casts in the code that don't make sense to
> > > me.  I think some of these were once necessary because char * was used
> > > in place of void * for some function arguments.  And some of these were
> > > probably just copied around without further thought.  I went through and
> > > cleaned up most of these.  I didn't find any redeeming value in these.
> > > They are just liable to hide actual problems such as incompatible types.
> > >    But maybe there are other opinions.
> > 
> > I don't recall details, but I'm fairly sure some of these prevented
> > compiler warnings on some (old?) compilers.  Hard to be sure if said
> > compilers are all gone.
> > 
> > Looking at the sheer size of the patch, I'm kind of -0.1, just
> > because I'm afraid it's going to create back-patching gotchas.
> > I don't really find that it's improving readability, though
> > clearly that's a matter of opinion.
> 
> I did a bit of archeological research on these.  None of these casts were
> ever necessary, and in many cases even the original patch that introduced an
> API used the coding style inconsistently.  So I'm very confident that there
> are no significant backward compatibility or backpatching gotchas here.
> 
> I'm more concerned that many of these just keep getting copied around
> indiscriminately, and this is liable to hide actual type mismatches or
> silently discard qualifiers.  So I'm arguing in favor of a more restrictive
> style in this matter.

I agree.  I realize this will cause backpatch complexities, but I think
removing these will be a net positive.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  When a patient asks the doctor, "Am I going to die?", he means 
  "Am I going to die soon?"