Remove useless casts to (void *)
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.
Attachments:
0001-Remove-useless-casts-to-void.patchtext/plain; charset=UTF-8; name=0001-Remove-useless-casts-to-void.patchDownload+493-552
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]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hake&dt=2024-12-02%2017%3A19%3A40&stg=make, 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
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
On Tue, Oct 29, 2024 at 9:06 PM Peter Eisentraut <peter@eisentraut.org> wrote:
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.
Some other places that sprang to my eye recently that reminded me of
K&R C and the ongoing transition to the standard (/me ducks):
Why do we bother with a "Pointer" type? The idiomatic way to do
byte-oriented pointer arithmetic is to cast to char *, or uint8_t*
etc, which doesn't require the reader to go and figure out what stride
that non-standard type name implies. Clearly it has a history linked
to the introduction of void *, and it's described as 'holding the
address of any memory resident type' and is filed under the 'standard
system types' section, but C89 has void * for objects of type unknown
to the compiler, and if you want to do arithmetic over those objects
you have to be more explicit about their size, which requires writing
a suitably-alarming-to-the-reader cast to a pointer to a real type.
Also, the explanation about "true ANSI compilers" means nothing to the
modern reader who wasn't hacking C 30+ years ago.
Maybe it has to do with matching up to DatumGetPointer().
DatumGetPointer() isn't defined to return it though, it returns char
*, callers usually/always cast to a real time, and I don't see Pointer
being used near it. DatumGetPointer() should arguably return void *
too, to force users to provide a type if they're going to dereference
or perform arithmetic.
What problem does PointerIsValid(x) solve, when you could literally
just write x or if you insist x != NULL in most contexts and it would
be 100% idiomatic C, and shorter? Why introduce extra terminological
load with "validity"? C programmers had better know all about NULL.
Why do all the XLogRegister*() calls have to cast their argument to
char *? Seems like a textbook use for void * argument, not requiring
a cast. If the XLog* interfaces want to do byte-oriented arithmetic
internally, they should do the cast internally, instead of having an
argument that requires all callers to cast their arguments.
(While grepping for casts to char *, I had a mistake in my regex and
finished up seeing how many places in our code check sizeof(char),
which is funny because sizeof is defined as telling you how many chars
it takes to hold a type/value; perhaps it has documentation value :-))
Thomas Munro <thomas.munro@gmail.com> writes:
Why do we bother with a "Pointer" type? The idiomatic way to do
byte-oriented pointer arithmetic is to cast to char *, or uint8_t*
etc, which doesn't require the reader to go and figure out what stride
that non-standard type name implies.
I think getting rid of Pointer altogether would cause a lot of code
churn for not that much benefit; most places are (I think) just using
it as a name for a generic pointer. I could support a patch to
define it as "void *" not "char *", if there aren't too many places
that would have to be modified.
DatumGetPointer() should arguably return void *
too, to force users to provide a type if they're going to dereference
or perform arithmetic.
Well, it returns Pointer, which is what it should return.
What problem does PointerIsValid(x) solve, when you could literally
just write x or if you insist x != NULL in most contexts and it would
be 100% idiomatic C, and shorter?
The same goes for a number of other historical macros, such as
OidIsValid. I think there was possibly once an idea that super-duper
debug builds could apply stricter tests in these macros. Nothing's
ever been done in that line, but that doesn't make it an awful idea.
I don't particularly object to code that just checks "if (x)", but
I wouldn't be in favor of removing these macros, if only because the
sheer size of the patch would make it a back-patching land mine.
Why do all the XLogRegister*() calls have to cast their argument to
char *? Seems like a textbook use for void * argument, not requiring
a cast.
Probably. Again, it'd be interesting to try changing it and see how
invasive the patch winds up being.
(While grepping for casts to char *, I had a mistake in my regex and
finished up seeing how many places in our code check sizeof(char),
which is funny because sizeof is defined as telling you how many chars
it takes to hold a type/value; perhaps it has documentation value :-))
Yeah. I think that "ptr = palloc(n * sizeof(char))" is good practice
as documentation, even when we all know "sizeof(char)" is 1.
regards, tom lane
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