Modernize const handling with readline
The comment
/* On some platforms, readline is declared as readline(char *) */
is obsolete. The casting away of const can be removed.
The const in the readline() prototype was added in GNU readline 4.2,
released in 2001. BSD libedit has also had const in the prototype since
at least 2001.
(The commit that introduced this comment (187e865174) talked about
FreeBSD 4.8, which didn't have readline compatibility in libedit yet, so
it must have been talking about GNU readline in the base system. This
checks out, but already FreeBSD 5 had an updated GNU readline with const.)
Attachments:
0001-Modernize-const-handling-with-readline.patchtext/plain; charset=UTF-8; name=0001-Modernize-const-handling-with-readline.patchDownload
From 4db9b54b40833486e366c2d117291147ce47b195 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Tue, 3 Oct 2023 12:08:25 +0200
Subject: [PATCH] Modernize const handling with readline
The comment
/* On some platforms, readline is declared as readline(char *) */
is obsolete. The casting away of const can be removed.
The const in the readline() prototype was added in GNU readline 4.2,
released in 2001. BSD libedit has also had const in the prototype
since at least 2001.
(The commit that introduced this comment (187e865174) talked about
FreeBSD 4.8, which didn't have readline compatibility in libedit yet,
so it must have been talking about GNU readline in the base system.
This checks out, but already FreeBSD 5 had an updated GNU readline
with const.)
---
src/bin/psql/input.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 873d85079c..7c77fd8e89 100644
--- a/src/bin/psql/input.c
+++ b/src/bin/psql/input.c
@@ -88,8 +88,7 @@ gets_interactive(const char *prompt, PQExpBuffer query_buf)
/* Enable SIGINT to longjmp to sigint_interrupt_jmp */
sigint_interrupt_enabled = true;
- /* On some platforms, readline is declared as readline(char *) */
- result = readline((char *) prompt);
+ result = readline(prompt);
/* Disable SIGINT again */
sigint_interrupt_enabled = false;
--
2.42.0
Hi,
The comment
/* On some platforms, readline is declared as readline(char *) */
is obsolete. The casting away of const can be removed.
The const in the readline() prototype was added in GNU readline 4.2,
released in 2001. BSD libedit has also had const in the prototype since
at least 2001.(The commit that introduced this comment (187e865174) talked about
FreeBSD 4.8, which didn't have readline compatibility in libedit yet, so
it must have been talking about GNU readline in the base system. This
checks out, but already FreeBSD 5 had an updated GNU readline with const.)
LGTM.
While examining the code for similar places I noticed that the
following functions can also be const'ified:
- crc32_sz
- pg_checksum_page (? temporary modifies the page but then restores it)
- XLogRegisterData (?)
The callers of cstring_to_text[_with_len] often cast the argument to
(char *) while in fact it's (const char *). This can be refactored
too.
Additionally there is a slight difference between XLogRegisterBlock()
declaration in xloginsert.h:
```
extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
ForkNumber forknum, BlockNumber blknum,
char *page,
uint8 flags);
```
... and xloginsert.c:
```
void
XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
BlockNumber blknum, Page page, uint8 flags)
```
Will there be a value in addressing anything of this?
--
Best regards,
Aleksander Alekseev
Peter Eisentraut <peter@eisentraut.org> writes:
The comment
/* On some platforms, readline is declared as readline(char *) */
is obsolete. The casting away of const can be removed.
+1, that's surely not of interest on anything we still support.
regards, tom lane
On 03.10.23 13:28, Aleksander Alekseev wrote:
While examining the code for similar places I noticed that the
following functions can also be const'ified:- crc32_sz
I suppose this could be changed.
- pg_checksum_page (? temporary modifies the page but then restores it)
Then it's not really const?
- XLogRegisterData (?)
I don't think this would work, at least without further work elsewhere,
because the data is stored in XLogRecData, which has no const handling.
The callers of cstring_to_text[_with_len] often cast the argument to
(char *) while in fact it's (const char *). This can be refactored
too.
These look fine to me.
Additionally there is a slight difference between XLogRegisterBlock()
declaration in xloginsert.h:```
extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
ForkNumber forknum, BlockNumber blknum,
char *page,
uint8 flags);
```... and xloginsert.c:
```
void
XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
BlockNumber blknum, Page page, uint8 flags)
```
It looks like the reason here is that xloginsert.h does not have the
Page type in scope. I don't know how difficult it would be to change
that, but it seems fine as is.
Hi,
On 03.10.23 13:28, Aleksander Alekseev wrote:
While examining the code for similar places I noticed that the
following functions can also be const'ified:- crc32_sz
I suppose this could be changed.
OK, that's a simple change. Here is the patch.
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-Constify-crc32_sz.patchapplication/octet-stream; name=v1-0001-Constify-crc32_sz.patchDownload
From 9184c3bb37d22bca5815c93fb9f627ed904bb5f4 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Wed, 4 Oct 2023 17:42:20 +0300
Subject: [PATCH v1] Constify crc32_sz
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: https://postgr.es/m/e08317a0-a2e7-c60d-c14a-ad9fc34f8f6c%40eisentraut.org
---
contrib/hstore/hstore_gist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/contrib/hstore/hstore_gist.c b/contrib/hstore/hstore_gist.c
index 3df00493e8..fe343739eb 100644
--- a/contrib/hstore/hstore_gist.c
+++ b/contrib/hstore/hstore_gist.c
@@ -77,7 +77,7 @@ typedef struct
/* shorthand for calculating CRC-32 of a single chunk of data. */
static pg_crc32
-crc32_sz(char *buf, int size)
+crc32_sz(const char *buf, int size)
{
pg_crc32 crc;
--
2.42.0
On 04.10.23 17:09, Aleksander Alekseev wrote:
Hi,
On 03.10.23 13:28, Aleksander Alekseev wrote:
While examining the code for similar places I noticed that the
following functions can also be const'ified:- crc32_sz
I suppose this could be changed.
OK, that's a simple change. Here is the patch.
committed this and my patch
On 04.10.23 16:37, Peter Eisentraut wrote:
On 03.10.23 13:28, Aleksander Alekseev wrote:
While examining the code for similar places I noticed that the
following functions can also be const'ified:
- XLogRegisterData (?)
I don't think this would work, at least without further work elsewhere,
because the data is stored in XLogRecData, which has no const handling.
I got around to fixing this. Here is a patch. It allows removing a few
unconstify() calls, which is nice.
Attachments:
0001-Add-const-qualifiers-to-XLogRegister-functions.patchtext/plain; charset=UTF-8; name=0001-Add-const-qualifiers-to-XLogRegister-functions.patchDownload
From aa59701d6904c0d21eebd4606f27bb0cf2c2b6f3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Wed, 28 Aug 2024 09:32:11 +0200
Subject: [PATCH] Add const qualifiers to XLogRegister*() functions
Add const qualifiers to XLogRegisterData() and XLogRegisterBufData().
Several unconstify() calls can be removed.
---
src/backend/access/brin/brin_pageops.c | 4 ++--
src/backend/access/transam/README | 4 ++--
src/backend/access/transam/xact.c | 4 ++--
src/backend/access/transam/xlog.c | 2 +-
src/backend/access/transam/xloginsert.c | 22 +++++++++++-----------
src/backend/replication/logical/message.c | 4 ++--
src/include/access/xlog_internal.h | 2 +-
src/include/access/xloginsert.h | 6 +++---
src/include/storage/bufpage.h | 4 ++--
9 files changed, 26 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
index b69217c1ec6..659936144ed 100644
--- a/src/backend/access/brin/brin_pageops.c
+++ b/src/backend/access/brin/brin_pageops.c
@@ -193,7 +193,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
XLogRegisterData((char *) &xlrec, SizeOfBrinSamepageUpdate);
XLogRegisterBuffer(0, oldbuf, REGBUF_STANDARD);
- XLogRegisterBufData(0, (char *) unconstify(BrinTuple *, newtup), newsz);
+ XLogRegisterBufData(0, (const char *) newtup, newsz);
recptr = XLogInsert(RM_BRIN_ID, info);
@@ -285,7 +285,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
XLogRegisterData((char *) &xlrec, SizeOfBrinUpdate);
XLogRegisterBuffer(0, newbuf, REGBUF_STANDARD | (extended ? REGBUF_WILL_INIT : 0));
- XLogRegisterBufData(0, (char *) unconstify(BrinTuple *, newtup), newsz);
+ XLogRegisterBufData(0, (const char *) newtup, newsz);
/* revmap page */
XLogRegisterBuffer(1, revmapbuf, 0);
diff --git a/src/backend/access/transam/README b/src/backend/access/transam/README
index 28d196cf62b..6e4711dace7 100644
--- a/src/backend/access/transam/README
+++ b/src/backend/access/transam/README
@@ -586,13 +586,13 @@ void XLogRegisterBuffer(uint8 block_id, Buffer buf, uint8 flags);
XLogRegisterBufData() is included in the WAL record even if a full-page
image is taken.
-void XLogRegisterData(char *data, int len);
+void XLogRegisterData(const char *data, int len);
XLogRegisterData is used to include arbitrary data in the WAL record. If
XLogRegisterData() is called multiple times, the data are appended, and
will be made available to the redo routine as one contiguous chunk.
-void XLogRegisterBufData(uint8 block_id, char *data, int len);
+void XLogRegisterBufData(uint8 block_id, const char *data, int len);
XLogRegisterBufData is used to include data associated with a particular
buffer that was registered earlier with XLogRegisterBuffer(). If
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index dfc8cf2dcf2..4eecbf12ed9 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -5951,7 +5951,7 @@ XactLogCommitRecord(TimestampTz commit_time,
{
XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
- XLogRegisterData(unconstify(char *, twophase_gid), strlen(twophase_gid) + 1);
+ XLogRegisterData(twophase_gid, strlen(twophase_gid) + 1);
}
if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)
@@ -6097,7 +6097,7 @@ XactLogAbortRecord(TimestampTz abort_time,
{
XLogRegisterData((char *) (&xl_twophase), sizeof(xl_xact_twophase));
if (xl_xinfo.xinfo & XACT_XINFO_HAS_GID)
- XLogRegisterData(unconstify(char *, twophase_gid), strlen(twophase_gid) + 1);
+ XLogRegisterData(twophase_gid, strlen(twophase_gid) + 1);
}
if (xl_xinfo.xinfo & XACT_XINFO_HAS_ORIGIN)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index ee0fb0e28f8..9744ba991bf 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1248,7 +1248,7 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
written = 0;
while (rdata != NULL)
{
- char *rdata_data = rdata->data;
+ const char *rdata_data = rdata->data;
int rdata_len = rdata->len;
while (rdata_len > freespace)
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 90476015347..f92d0626082 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -72,7 +72,7 @@ typedef struct
RelFileLocator rlocator; /* identifies the relation and block */
ForkNumber forkno;
BlockNumber block;
- Page page; /* page content */
+ const char *page; /* page content */
uint32 rdata_len; /* total length of data in rdata chain */
XLogRecData *rdata_head; /* head of the chain of data registered with
* this block */
@@ -138,7 +138,7 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr RedoRecPtr, bool doPageWrites,
XLogRecPtr *fpw_lsn, int *num_fpi,
bool *topxid_included);
-static bool XLogCompressBackupBlock(char *page, uint16 hole_offset,
+static bool XLogCompressBackupBlock(const char *page, uint16 hole_offset,
uint16 hole_length, char *dest, uint16 *dlen);
/*
@@ -307,7 +307,7 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags)
*/
void
XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
- BlockNumber blknum, Page page, uint8 flags)
+ BlockNumber blknum, const char *page, uint8 flags)
{
registered_buffer *regbuf;
@@ -361,7 +361,7 @@ XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
* XLogRecGetData().
*/
void
-XLogRegisterData(char *data, uint32 len)
+XLogRegisterData(const char *data, uint32 len)
{
XLogRecData *rdata;
@@ -402,7 +402,7 @@ XLogRegisterData(char *data, uint32 len)
* limited)
*/
void
-XLogRegisterBufData(uint8 block_id, char *data, uint32 len)
+XLogRegisterBufData(uint8 block_id, const char *data, uint32 len)
{
registered_buffer *regbuf;
XLogRecData *rdata;
@@ -648,7 +648,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
if (include_image)
{
- Page page = regbuf->page;
+ const char *page = regbuf->page;
uint16 compressed_len = 0;
/*
@@ -941,23 +941,23 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
* the length of compressed block image.
*/
static bool
-XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
+XLogCompressBackupBlock(const char *page, uint16 hole_offset, uint16 hole_length,
char *dest, uint16 *dlen)
{
int32 orig_len = BLCKSZ - hole_length;
int32 len = -1;
int32 extra_bytes = 0;
- char *source;
+ const char *source;
PGAlignedBlock tmp;
if (hole_length != 0)
{
/* must skip the hole */
- source = tmp.data;
- memcpy(source, page, hole_offset);
- memcpy(source + hole_offset,
+ memcpy(tmp.data, page, hole_offset);
+ memcpy(tmp.data + hole_offset,
page + (hole_offset + hole_length),
BLCKSZ - (hole_length + hole_offset));
+ source = tmp.data;
/*
* Extra data needs to be stored in WAL record for the compressed
diff --git a/src/backend/replication/logical/message.c b/src/backend/replication/logical/message.c
index 9e41aac2813..26fca3a8e5c 100644
--- a/src/backend/replication/logical/message.c
+++ b/src/backend/replication/logical/message.c
@@ -63,8 +63,8 @@ LogLogicalMessage(const char *prefix, const char *message, size_t size,
XLogBeginInsert();
XLogRegisterData((char *) &xlrec, SizeOfLogicalMessage);
- XLogRegisterData(unconstify(char *, prefix), xlrec.prefix_size);
- XLogRegisterData(unconstify(char *, message), size);
+ XLogRegisterData(prefix, xlrec.prefix_size);
+ XLogRegisterData(message, size);
/* allow origin filtering */
XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index c6a91fb4560..e5cdba0584a 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -312,7 +312,7 @@ typedef struct xl_end_of_recovery
typedef struct XLogRecData
{
struct XLogRecData *next; /* next struct in chain, or NULL */
- char *data; /* start of rmgr data to include */
+ const char *data; /* start of rmgr data to include */
uint32 len; /* length of rmgr data to include */
} XLogRecData;
diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h
index b44fa29eac5..652f7bc9bd1 100644
--- a/src/include/access/xloginsert.h
+++ b/src/include/access/xloginsert.h
@@ -44,12 +44,12 @@ extern void XLogBeginInsert(void);
extern void XLogSetRecordFlags(uint8 flags);
extern XLogRecPtr XLogInsert(RmgrId rmid, uint8 info);
extern void XLogEnsureRecordSpace(int max_block_id, int ndatas);
-extern void XLogRegisterData(char *data, uint32 len);
+extern void XLogRegisterData(const char *data, uint32 len);
extern void XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags);
extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
- ForkNumber forknum, BlockNumber blknum, char *page,
+ ForkNumber forknum, BlockNumber blknum, const char *page,
uint8 flags);
-extern void XLogRegisterBufData(uint8 block_id, char *data, uint32 len);
+extern void XLogRegisterBufData(uint8 block_id, const char *data, uint32 len);
extern void XLogResetInsertion(void);
extern bool XLogCheckBufferNeedsBackup(Buffer buffer);
diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h
index 5999e5ca5a5..6222d46e535 100644
--- a/src/include/storage/bufpage.h
+++ b/src/include/storage/bufpage.h
@@ -384,9 +384,9 @@ PageGetMaxOffsetNumber(Page page)
* Additional functions for access to page headers.
*/
static inline XLogRecPtr
-PageGetLSN(Page page)
+PageGetLSN(const char *page)
{
- return PageXLogRecPtrGet(((PageHeader) page)->pd_lsn);
+ return PageXLogRecPtrGet(((const PageHeaderData *) page)->pd_lsn);
}
static inline void
PageSetLSN(Page page, XLogRecPtr lsn)
base-commit: 6654bb92047b37cee053cedd6fa1829841b2ad8e
--
2.46.0
Hi,
On 04.10.23 16:37, Peter Eisentraut wrote:
On 03.10.23 13:28, Aleksander Alekseev wrote:
While examining the code for similar places I noticed that the
following functions can also be const'ified:- XLogRegisterData (?)
I don't think this would work, at least without further work elsewhere,
because the data is stored in XLogRecData, which has no const handling.I got around to fixing this. Here is a patch. It allows removing a few
unconstify() calls, which is nice.
LGTM.
Note that this may affect third-party code. IMO this is not a big deal
in this particular case.
Also by randomly checking one of the affected non-static functions I
found a bunch of calls like this:
XLogRegisterData((char *) msgs, ...)
... where the first argument is going to become (const char *). It
looks like the compilers are OK with implicitly casting (char*) to a
more restrictive (const char*) though.
--
Best regards,
Aleksander Alekseev
On 28.08.24 12:04, Aleksander Alekseev wrote:
Hi,
On 04.10.23 16:37, Peter Eisentraut wrote:
On 03.10.23 13:28, Aleksander Alekseev wrote:
While examining the code for similar places I noticed that the
following functions can also be const'ified:- XLogRegisterData (?)
I don't think this would work, at least without further work elsewhere,
because the data is stored in XLogRecData, which has no const handling.I got around to fixing this. Here is a patch. It allows removing a few
unconstify() calls, which is nice.LGTM.
committed
Note that this may affect third-party code. IMO this is not a big deal
in this particular case.
I don't think this will impact any third-party code. Only maybe for the
better, by being able to remove some casts.
Also by randomly checking one of the affected non-static functions I
found a bunch of calls like this:XLogRegisterData((char *) msgs, ...)
... where the first argument is going to become (const char *). It
looks like the compilers are OK with implicitly casting (char*) to a
more restrictive (const char*) though.
Yes, that's ok.