Thread: Re: Remove useless casts to (void *)
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
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?"
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.
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?"
On 11/14/24 9:59 AM, Peter Eisentraut wrote: > 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. +1 I agree. This is likely to hide real issues. Andreas
On 28.11.24 04:54, Andreas Karlsson wrote: > On 11/14/24 9:59 AM, Peter Eisentraut wrote: >> 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. > > +1 I agree. This is likely to hide real issues. Committed, thanks.
Peter Eisentraut <peter@eisentraut.org> writes:
> Committed, thanks.
Now that we have a more-or-less full set of buildfarm results
on this, I checked for new warnings, and found two:
pg_shmem.c: In function 'PGSharedMemoryIsInUse':
pg_shmem.c:323:33: warning: passing argument 1 of 'shmdt' from incompatible pointer type [-Wincompatible-pointer-types]
323 | if (memAddress && shmdt(memAddress) < 0)
| ^~~~~~~~~~
| |
| PGShmemHeader *
In file included from pg_shmem.c:27:
/usr/include/sys/shm.h:131:11: note: expected 'char *' but argument is of type 'PGShmemHeader *'
131 | int shmdt(char *);
| ^~~~~~
pg_shmem.c: In function 'PGSharedMemoryCreate':
pg_shmem.c:838:37: warning: passing argument 1 of 'shmdt' from incompatible pointer type [-Wincompatible-pointer-types]
838 | if (oldhdr && shmdt(oldhdr) < 0)
| ^~~~~~
| |
| PGShmemHeader *
/usr/include/sys/shm.h:131:11: note: expected 'char *' but argument is of type 'PGShmemHeader *'
131 | int shmdt(char *);
| ^~~~~~
This is from hake[1], which is running OpenIndiana/illumos.
That platform shows a couple of other strange warnings, so maybe
re-eliminating these two is not worth worrying about, but
nonetheless the casts to void * were doing something here.
regards, tom lane
[1] https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hake&dt=2024-12-02%2017%3A19%3A40&stg=make
On Tue, Dec 3, 2024 at 7:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > This is from hake[1], which is running OpenIndiana/illumos. > That platform shows a couple of other strange warnings, so maybe > re-eliminating these two is not worth worrying about, but > nonetheless the casts to void * were doing something here. I wouldn't change that. illumos is selecting the old pre-standard declaration here, but it knows the standard one: https://github.com/illumos/illumos-gate/blob/27ecbff00d8c86a2647d6fe325cacb220d712115/usr/src/uts/common/sys/shm.h#L129 https://illumos.org/man/2/shmdt I don't know why we have only one tiny issue if the system headers think we want pre-POSIX stuff. I wonder if the particular header has incorrect guarding, but I don't know how that is supposed to work.
Thomas Munro <thomas.munro@gmail.com> writes: > I wouldn't change that. illumos is selecting the old pre-standard > declaration here, but it knows the standard one: > https://github.com/illumos/illumos-gate/blob/27ecbff00d8c86a2647d6fe325cacb220d712115/usr/src/uts/common/sys/shm.h#L129 > https://illumos.org/man/2/shmdt Oh! Kind of looks like we should be defining _POSIX_C_SOURCE=200112L, at least on that platform. > I don't know why we have only one tiny issue if the system headers > think we want pre-POSIX stuff. Agreed, I'd have expected more trouble than this. But persuading the system headers that we want a POSIX version from this century seems like it might be a good idea to forestall future issues. I'm inclined to propose adding something like CPPFLAGS="$CPPFLAGS -D_POSIX_C_SOURCE=200112L" to src/template/solaris. Not sure if we have a corresponding mechanism for meson, though. regards, tom lane
Hi, On 2024-12-02 17:11:30 -0500, Tom Lane wrote: > I'm inclined to propose adding something like > > CPPFLAGS="$CPPFLAGS -D_POSIX_C_SOURCE=200112L" > > to src/template/solaris. Not sure if we have a corresponding > mechanism for meson, though. elif host_system == 'sunos' portname = 'solaris' export_fmt = '-Wl,-M@0@' cppflags += '-D_POSIX_PTHREAD_SEMANTICS' Should be trivial to add there. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2024-12-02 17:11:30 -0500, Tom Lane wrote:
>> I'm inclined to propose adding something like
>> CPPFLAGS="$CPPFLAGS -D_POSIX_C_SOURCE=200112L"
>> to src/template/solaris. Not sure if we have a corresponding
>> mechanism for meson, though.
> elif host_system == 'sunos'
> portname = 'solaris'
> export_fmt = '-Wl,-M@0@'
> cppflags += '-D_POSIX_PTHREAD_SEMANTICS'
> Should be trivial to add there.
Oh! The corresponding bit in configure.ac is
# On Solaris, we need this #define to get POSIX-conforming versions
# of many interfaces (sigwait, getpwuid_r, ...).
if test "$PORTNAME" = "solaris"; then
CPPFLAGS="$CPPFLAGS -D_POSIX_PTHREAD_SEMANTICS"
fi
Barely even need to adjust the comment ;-). I'll proceed with
improving that (in master only, don't think we need it in back
branches, at least not today) unless somebody pushes back soon.
regards, tom lane
On 2024-12-02 17:42:33 -0500, Tom Lane wrote: > I'll proceed with improving that (in master only, don't think we need it in > back branches, at least not today) unless somebody pushes back soon. +1
Andres Freund <andres@anarazel.de> writes:
> On 2024-12-02 17:42:33 -0500, Tom Lane wrote:
>> I'll proceed with improving that (in master only, don't think we need it in
>> back branches, at least not today) unless somebody pushes back soon.
> +1
Pushed; I'll await hake's next run with interest.
regards, tom lane
I wrote: > Pushed; I'll await hake's next run with interest. hake didn't like that, but after adding -D__EXTENSIONS__ per https://illumos.org/man/7/standards it seems happy again. Its configure results are the same as beforehand, and the warning about shmdt() is gone. regards, tom lane
On Wed, Dec 4, 2024 at 5:33 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > hake didn't like that, but after adding -D__EXTENSIONS__ per > https://illumos.org/man/7/standards > it seems happy again. Its configure results are the same as > beforehand, and the warning about shmdt() is gone. Cool. I was wondering if it was going to break on some of our recent POSIX 2008 stuff (thread-safe <locale.h> bits and pieces) next, given _POSIX_C_SOURCE=200112L. It certainly does know about 2008 too, so it looks like the man page might be out of date. https://github.com/illumos/illumos-gate/blob/master/usr/src/boot/sys/sys/cdefs.h#L724
Thomas Munro <thomas.munro@gmail.com> writes:
> Cool. I was wondering if it was going to break on some of our recent
> POSIX 2008 stuff (thread-safe <locale.h> bits and pieces) next, given
> _POSIX_C_SOURCE=200112L. It certainly does know about 2008 too, so it
> looks like the man page might be out of date.
Do you want to try setting it to 200809? But let's wait to see what
margay has to say about the current choices.
regards, tom lane