Remove useless casts to (char *)
In the spirit of the recent patch set "Remove useless casts to (void *)"
[0]: /messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org
to (char *).
There are two larger themes:
1) Various casts around string/memory functions such as strcpy() or
memcpy() that pretty much don't make any sense at all (at least
post-1989 I guess).
2) Using void * instead of char * for function arguments that deal with
binary data. The largest of these is XLogRegisterData() and
XLogRegisterBufData(), which were also mentioned in [0]/messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org. (similar past
patches: 2d4f1ba6cfc 1f605b82ba6 3b12e68a5c4 b9f0e54bc95)
The remaining (char *) casts are mostly related to signed/unsigned
conversion, controlling pointer arithmetic, and related to
palloc/malloc, (and probably some I missed or didn't want to touch) so
those are all ok to keep.
[0]: /messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org
/messages/by-id/461ea37c-8b58-43b4-9736-52884e862820@eisentraut.org
Attachments:
0001-Remove-unnecessary-char-casts-string.patchtext/plain; charset=UTF-8; name=0001-Remove-unnecessary-char-casts-string.patchDownload+18-19
0002-Remove-unnecessary-char-casts-mem.patchtext/plain; charset=UTF-8; name=0002-Remove-unnecessary-char-casts-mem.patchDownload+44-49
0003-Remove-unnecessary-char-casts-checksum.patchtext/plain; charset=UTF-8; name=0003-Remove-unnecessary-char-casts-checksum.patchDownload+5-6
0004-Remove-various-unnecessary-char-casts.patchtext/plain; charset=UTF-8; name=0004-Remove-various-unnecessary-char-casts.patchDownload+35-34
0005-backend-launchers-void-arguments-for-binary-data.patchtext/plain; charset=UTF-8; name=0005-backend-launchers-void-arguments-for-binary-data.patchDownload+42-43
0006-backend-libpq-void-argument-for-binary-data.patchtext/plain; charset=UTF-8; name=0006-backend-libpq-void-argument-for-binary-data.patchDownload+18-16
0007-jsonb-internal-API-void-argument-for-binary-data.patchtext/plain; charset=UTF-8; name=0007-jsonb-internal-API-void-argument-for-binary-data.patchDownload+10-11
0008-SnapBuildRestoreContents-void-argument-for-binary-da.patchtext/plain; charset=UTF-8; name=0008-SnapBuildRestoreContents-void-argument-for-binary-da.patchDownload+6-7
0009-XLogRegisterData-XLogRegisterBufData-void-argument-f.patchtext/plain; charset=UTF-8; name=0009-XLogRegisterData-XLogRegisterBufData-void-argument-f.patchDownload+7-8
0010-Remove-unnecessary-char-casts-xlog.patchtext/plain; charset=UTF-8; name=0010-Remove-unnecessary-char-casts-xlog.patchDownload+234-235
Hi Peter,
I have only skimmed the patches, but one hunk jumped out at me:
Peter Eisentraut <peter@eisentraut.org> writes:
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 1bf27d93cfa..937a2b02a4f 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -1368,7 +1368,7 @@ internal_flush_buffer(const char *buf, size_t *start, size_t *end) { int r;- r = secure_write(MyProcPort, (char *) bufptr, bufend - bufptr); + r = secure_write(MyProcPort, unconstify(char *, bufptr), bufend - bufptr);if (r <= 0)
{
Insted of unconstify here, could we not make secure_write() (and
be_tls_write()) take the buffer pointer as const void *, like the
attached?
- ilmari
Attachments:
0001-Make-TLS-write-functions-buffe-arguments-pointers-to.txttext/x-patchDownload+5-6
On 06.02.25 12:49, Dagfinn Ilmari Mannsåker wrote:
I have only skimmed the patches, but one hunk jumped out at me:
Peter Eisentraut <peter@eisentraut.org> writes:
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 1bf27d93cfa..937a2b02a4f 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -1368,7 +1368,7 @@ internal_flush_buffer(const char *buf, size_t *start, size_t *end) { int r;- r = secure_write(MyProcPort, (char *) bufptr, bufend - bufptr); + r = secure_write(MyProcPort, unconstify(char *, bufptr), bufend - bufptr);if (r <= 0)
{Insted of unconstify here, could we not make secure_write() (and
be_tls_write()) take the buffer pointer as const void *, like the
attached?
Yeah, that makes sense. I've committed that. Here is a new patch set
rebased over that.
Attachments:
v2-0001-Remove-unnecessary-char-casts-string.patchtext/plain; charset=UTF-8; name=v2-0001-Remove-unnecessary-char-casts-string.patchDownload+18-19
v2-0002-Remove-unnecessary-char-casts-mem.patchtext/plain; charset=UTF-8; name=v2-0002-Remove-unnecessary-char-casts-mem.patchDownload+44-49
v2-0003-Remove-unnecessary-char-casts-checksum.patchtext/plain; charset=UTF-8; name=v2-0003-Remove-unnecessary-char-casts-checksum.patchDownload+5-6
v2-0004-Remove-various-unnecessary-char-casts.patchtext/plain; charset=UTF-8; name=v2-0004-Remove-various-unnecessary-char-casts.patchDownload+34-33
v2-0005-backend-launchers-void-arguments-for-binary-data.patchtext/plain; charset=UTF-8; name=v2-0005-backend-launchers-void-arguments-for-binary-data.patchDownload+42-43
v2-0006-backend-libpq-void-argument-for-binary-data.patchtext/plain; charset=UTF-8; name=v2-0006-backend-libpq-void-argument-for-binary-data.patchDownload+18-16
v2-0007-jsonb-internal-API-void-argument-for-binary-data.patchtext/plain; charset=UTF-8; name=v2-0007-jsonb-internal-API-void-argument-for-binary-data.patchDownload+10-11
v2-0008-SnapBuildRestoreContents-void-argument-for-binary.patchtext/plain; charset=UTF-8; name=v2-0008-SnapBuildRestoreContents-void-argument-for-binary.patchDownload+6-7
v2-0009-XLogRegisterData-XLogRegisterBufData-void-argumen.patchtext/plain; charset=UTF-8; name=v2-0009-XLogRegisterData-XLogRegisterBufData-void-argumen.patchDownload+7-8
v2-0010-Remove-unnecessary-char-casts-xlog.patchtext/plain; charset=UTF-8; name=v2-0010-Remove-unnecessary-char-casts-xlog.patchDownload+234-235
Peter Eisentraut <peter@eisentraut.org> writes:
On 06.02.25 12:49, Dagfinn Ilmari Mannsåker wrote:
I have only skimmed the patches, but one hunk jumped out at me:
Peter Eisentraut <peter@eisentraut.org> writes:diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c index 1bf27d93cfa..937a2b02a4f 100644 --- a/src/backend/libpq/pqcomm.c +++ b/src/backend/libpq/pqcomm.c @@ -1368,7 +1368,7 @@ internal_flush_buffer(const char *buf, size_t *start, size_t *end) { int r; - r = secure_write(MyProcPort, (char *) bufptr, bufend - bufptr); + r = secure_write(MyProcPort, unconstify(char *, bufptr), bufend - bufptr); if (r <= 0) {Insted of unconstify here, could we not make secure_write() (and
be_tls_write()) take the buffer pointer as const void *, like the
attached?Yeah, that makes sense. I've committed that.
Thanks, and thanks for catching be_gssapi_write(), which I had missed
due to not having gssapi enabled in my test build.
Here is a new patch set rebased over that.
I had a more thorough read-through this time (as well as applying and
building it), and it does make the code a lot more readable.
I noticed you in some places added extra parens around remaining casts
with offset additions, e.g.
- XLogRegisterData((char *) old_key_tuple->t_data + SizeofHeapTupleHeader,
+ XLogRegisterData(((char *) old_key_tuple->t_data) + SizeofHeapTupleHeader,
old_key_tuple->t_len - SizeofHeapTupleHeader);But not in others:
- memcpy((char *) tuple->t_data + SizeofHeapTupleHeader,
- (char *) data,
- datalen);
+ memcpy((char *) tuple->t_data + SizeofHeapTupleHeader, data, datalen);I don't have a particularly strong opinion either way (maybe -0.2 on the
extra parens), but I mainly think we should keep it consistent, and not
change it gratuitously.
Greppig indicates to me that the paren-less version is more common:
$ git grep -P '\(\w+\s*\**\) [\w>-]+ \+ \w+' | wc -l
283
$ git grep -P '\(\(\w+\s*\**\) [\w>-]+\) \+ \w+' | wc -l
96
So I think we should leave them as they are.
- ilmari
I have committed the rest of this with the adjustments you suggested.
Show quoted text
On 10.02.25 18:44, Dagfinn Ilmari Mannsåker wrote:
Here is a new patch set rebased over that.
I had a more thorough read-through this time (as well as applying and
building it), and it does make the code a lot more readable.I noticed you in some places added extra parens around remaining casts
with offset additions, e.g.- XLogRegisterData((char *) old_key_tuple->t_data + SizeofHeapTupleHeader, + XLogRegisterData(((char *) old_key_tuple->t_data) + SizeofHeapTupleHeader, old_key_tuple->t_len - SizeofHeapTupleHeader);But not in others:
- memcpy((char *) tuple->t_data + SizeofHeapTupleHeader, - (char *) data, - datalen); + memcpy((char *) tuple->t_data + SizeofHeapTupleHeader, data, datalen);I don't have a particularly strong opinion either way (maybe -0.2 on the
extra parens), but I mainly think we should keep it consistent, and not
change it gratuitously.Greppig indicates to me that the paren-less version is more common:
$ git grep -P '\(\w+\s*\**\) [\w>-]+ \+ \w+' | wc -l
283
$ git grep -P '\(\(\w+\s*\**\) [\w>-]+\) \+ \w+' | wc -l
96So I think we should leave them as they are.
- ilmari
Peter Eisentraut писал(а) 2025-02-23 21:23:
I have committed the rest of this with the adjustments you suggested.
On 10.02.25 18:44, Dagfinn Ilmari Mannsåker wrote:
Here is a new patch set rebased over that.
Hi
I mentioned this patch in my message
/messages/by-id/f28f3b45ec84bf9dc99fe129023a2d6b@postgrespro.ru
Starting from it queries with Parallel Seq Scan (probably with other
parallel executor nodes)
hang under the debugger in Linux and MacOs.
--
Best regards,
Vladlen Popolitov.
On Wed, Mar 26, 2025 at 10:01:47PM +0700, Vladlen Popolitov wrote:
I mentioned this patch in my message /messages/by-id/f28f3b45ec84bf9dc99fe129023a2d6b@postgrespro.ru
Starting from it queries with Parallel Seq Scan (probably with other
parallel executor nodes)
hang under the debugger in Linux and MacOs.
Uh. How could a simple cast impact a parallel seqscan? That seems
unrelated to me.
--
Michael
Michael Paquier писал(а) 2025-03-27 05:20:
On Wed, Mar 26, 2025 at 10:01:47PM +0700, Vladlen Popolitov wrote:
I mentioned this patch in my message
/messages/by-id/f28f3b45ec84bf9dc99fe129023a2d6b@postgrespro.ru
Starting from it queries with Parallel Seq Scan (probably with other
parallel executor nodes)
hang under the debugger in Linux and MacOs.Uh. How could a simple cast impact a parallel seqscan? That seems
unrelated to me.
--
Michael
Hi Michel,
I changed the debugging extension in VScode and this issue disappeared.
I am sorry for disturbing and taking your time.
--
Best regards,
Vladlen Popolitov.