Re: more unconstify use
On 13/02/2019 19:51, Mark Dilger wrote: > Peter, so sorry I did not review this patch before you committed. There > are a few places where you unconstify the argument to a function where > changing the function to take const seems better to me. I argued for > something similar in 2016, One can consider unconstify a "todo" marker. Some of these could be removed by some API changes. It needs case-by-case consideration. > Your change: > > - md5_calc((uint8 *) (input + i), ctxt); > + md5_calc(unconstify(uint8 *, (input + i)), ctxt); > > Perhaps md5_calc's signature should change to > > md5_calc(const uint8 *, md5_ctxt *) > > since it doesn't modify the first argument. Fixed, thanks. > Your change: > > - if (!PageIndexTupleOverwrite(oldpage, oldoff, (Item) newtup, > newsz)) > + if (!PageIndexTupleOverwrite(oldpage, oldoff, (Item) > unconstify(BrinTuple *, newtup), newsz)) > > Perhaps PageIndexTupleOverwrite's third argument should be const, since it > only uses it as the const source of a memcpy. (This is a bit harder than > for md5_calc, above, since the third argument here is of type Item, which > is itself a typedef to Pointer, and there exists no analogous ConstPointer > or ConstItem definition in the sources.) Yeah, typedefs to a pointer are a poor fit for this. We have been getting rid of them from time to time, but I don't know a general solution. > Your change: > > - XLogRegisterBufData(0, (char *) newtup, newsz); > + XLogRegisterBufData(0, (char *) unconstify(BrinTuple *, > newtup), newsz); > > Perhaps the third argument to XLogRegisterBufData can be changed to const, > with the extra work of changing XLogRecData's data field to const. I'm not > sure about the amount of code churn this would entail. IIRC, the XLogRegister stuff is a web of lies with respect to constness. Resolving this properly is tricky. > Your change: > > - result = pg_be_scram_exchange(scram_opaq, input, inputlen, > + result = pg_be_scram_exchange(scram_opaq, unconstify(char *, > input), inputlen, > > , , > > logdetail); > > pg_be_scram_exchange passes the second argument into two functions, > read_client_first_message and read_client_final_message, both of which take > it as a non-const argument but immediately pstrdup it such that it might > as well have been const. Perhaps we should just clean up this mess rather > than unconstifying. Also fixed! -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: more unconstify use
> On Feb 7, 2019, at 12:14 AM, Peter Eisentraut > wrote: > > Here is a patch that makes more use of unconstify() by replacing casts > whose only purpose is to cast away const. Also a preliminary patch to > remove casts that were useless to begin with. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services > Peter, so sorry I did not review this patch before you committed. There are a few places where you unconstify the argument to a function where changing the function to take const seems better to me. I argued for something similar in 2016, https://www.postgresql.org/message-id/acf3a030-e3d5-4e68-b744-184e11de6...@gmail.com Back then, Tom replied > > I'd call this kind of a wash, I guess. I'd be more excited about it if > the change allowed removal of an actual cast-away-of-constness somewhere. We'd be able to remove some of these unconstify calls, and perhaps that meets Tom's criteria from that thread. Your change: - md5_calc((uint8 *) (input + i), ctxt); + md5_calc(unconstify(uint8 *, (input + i)), ctxt); Perhaps md5_calc's signature should change to md5_calc(const uint8 *, md5_ctxt *) since it doesn't modify the first argument. Your change: - if (!PageIndexTupleOverwrite(oldpage, oldoff, (Item) newtup, newsz)) + if (!PageIndexTupleOverwrite(oldpage, oldoff, (Item) unconstify(BrinTuple *, newtup), newsz)) Perhaps PageIndexTupleOverwrite's third argument should be const, since it only uses it as the const source of a memcpy. (This is a bit harder than for md5_calc, above, since the third argument here is of type Item, which is itself a typedef to Pointer, and there exists no analogous ConstPointer or ConstItem definition in the sources.) Your change: - XLogRegisterBufData(0, (char *) newtup, newsz); + XLogRegisterBufData(0, (char *) unconstify(BrinTuple *, newtup), newsz); Perhaps the third argument to XLogRegisterBufData can be changed to const, with the extra work of changing XLogRecData's data field to const. I'm not sure about the amount of code churn this would entail. Your change: - newoff = PageAddItem(newpage, (Item) newtup, newsz, + newoff = PageAddItem(newpage, (Item) unconstify(BrinTuple *, newtup), newsz, InvalidOffsetNumber, false, false); As with PageIndexTupleOverwrite's third argument, the second argument to PageAddItem is only ever used as the const source of a memcpy. Your change: - XLogRegisterData((char *) twophase_gid, strlen(twophase_gid) + 1); + XLogRegisterData(unconstify(char *, twophase_gid), strlen(twophase_gid) + 1); The first argument here gets assigned to XLogRecData.data, similarly to what is done above in XLogRegisterBufData. This is another place where casting away const could be avoided if we accepted the code churn cost of changing XLogRecData's data field to const. There are a few more of these in your patch which for brevity I won't quote. Your change: - result = pg_be_scram_exchange(scram_opaq, input, inputlen, + result = pg_be_scram_exchange(scram_opaq, unconstify(char *, input), inputlen, , , logdetail); pg_be_scram_exchange passes the second argument into two functions, read_client_first_message and read_client_final_message, both of which take it as a non-const argument but immediately pstrdup it such that it might as well have been const. Perhaps we should just clean up this mess rather than unconstifying. mark
Re: more unconstify use
On 07/02/2019 09:14, Peter Eisentraut wrote: > Here is a patch that makes more use of unconstify() by replacing casts > whose only purpose is to cast away const. Also a preliminary patch to > remove casts that were useless to begin with. committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
more unconstify use
*
get_code_entry(pg_wchar code)
{
return bsearch(&(code),
- (void *) UnicodeDecompMain,
+ UnicodeDecompMain,
lengthof(UnicodeDecompMain),
sizeof(pg_unicode_decomposition),
conv_compare);
base-commit: 793c736d69091d385a967b2740cc93cfb7a7b076
--
2.20.1
From 8dc004406f742dce9c76d0b922f45fdfda174ea3 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 29 Jan 2019 01:16:24 +0100
Subject: [PATCH v1 2/5] More unconstify use
Replace casts whose only purpose is to cast away const with the
unconstify() macro.
---
contrib/btree_gist/btree_utils_num.c | 8
contrib/pgcrypto/imath.c | 2 +-
contrib/pgcrypto/md5.c| 2 +-
contrib/pgcrypto/pgp-compress.c | 2 +-
src/backend/access/brin/brin_pageops.c| 8
src/backend/access/transam/xact.c | 4 ++--
src/backend/executor/spi.c| 10 +-
src/backend/libpq/auth.c | 10 +-
src/backend/libpq/be-secure-openssl.c | 2 +-
src/backend/parser/parse_type.c | 2 +-
src/backend/replication/logical/message.c | 4 ++--
src/backend/tcop/postgres.c | 2 +-
src/backend/utils/adt/formatting.c| 2 +-
src/backend/utils/mb/mbutils.c| 20 ++--
src/backend/utils/misc/guc.c | 2 +-
src/bin/pg_basebackup/pg_basebackup.c | 2 +-
src/bin/pg_basebackup/walmethods.c| 2 +-
src/bin/pg_dump/compress_io.c | 2 +-
src/bin/psql/prompt.c | 2 +-
src/fe_utils/print.c | 2 +-
src/interfaces/libpq/fe-lobj.c| 2 +-
src/interfaces/libpq/pqexpbuffer.c| 8 +---
src/pl/tcl/pltcl.c| 4 ++--
src/port/path.c | 6 +++---
src/timezone/localtime.c | 2 +-
25 files changed, 57 insertions(+), 55 deletions(-)
diff --git a/contrib/btree_gist/btree_utils_num.c
b/contrib/btree_gist/btree_utils_num.c
index 4d10bc93f3..7564a403c7 100644
--- a/contrib/btree_gist/btree_utils_num.c
+++ b/contrib/btree_gist/btree_utils_num.c
@@ -185,10 +185,10 @@ gbt_num_union(GBT_NUMKEY *out, const GistEntryVector
*entryvec, const gbtree_nin
c.upper = [tinfo->size];
/* if out->lower > cur->lower, adopt cur as lower */
if (tinfo->f_gt(o.lower, c.lower, flinfo))
- memcpy((void *) o.lower, c.lower, tinfo->size);
+ memcpy(unconstify(GBT_NUMKEY *, o.lower), c.lower,
tinfo->size);
/* if out->upper < cur->upper, adopt cur as upper */
if (tinfo->f_lt(o.upper, c.upper, flinfo))
- memcpy((void *) o.upper, c.upper, tinfo->size);
+ memcpy(unconstify(GBT_NUMKEY *, o.upper), c.upper,
tinfo->size);
}
return out;
@@ -237,9 +237,9 @@ gbt_num_bin_union(Datum *u, GBT_NUMKEY *e, const
gbtree_ninfo *tinfo, FmgrInfo *
ur.lower = &(((GBT_NUMKEY *) DatumGetPointer(*u))[0]);
ur.upper = &(((GBT_NUMKEY *) DatumGetPointer(*u))[tinfo->size]);
if (tinfo->f_gt(ur.lower, rd.lower, flinfo))
- memcpy((void *) ur.lower, rd.lower, tinfo->size);
+ memcpy(unconstify(GBT_NUMKEY *, ur.lower), rd.lower,
tinfo->size);
if (tinfo->f_lt(ur.upper, rd.upper, flinfo))
- memcpy((void *) ur.upper, rd.upper, tinfo->size);
+ memcpy(unconstify(GBT_NUMKEY *, ur.upper), rd.upper,
tinfo->size);
}
}
diff --git a/contrib/pgcrypto/imath.c b/contrib/pgcrypto/imath.c
index b94a51b81a..c267ea6ab3 100644
--- a/contrib/pgcrypto/imath.c
+++ b/contrib/pgcrypto/imath.c
@@ -2050,7 +2050,7 @@ mp_int_read_cstring(mp_int z, mp_size radix, const char
*str, char **end)
MP_SIGN(z) = MP_ZPOS;
if (end != NULL)
- *end = (char *) str;
+ *end = unconstify(char *, str);
/*
* Return a truncation error if the string has unprocessed characters
diff --git a/contrib/pgcrypto/md5.c b/contrib/pgcrypto/md5.c
index cac4e408ab..5bcfbfe28f 100644
--- a/contrib/pgcrypto/md5.c
+++ b/contrib/pgcrypto/md5.c
@@ -161,7 +161,7 @@ md5_loop(md5_ctxt *ctxt, const uint8 *input, unsigned len)
md5_calc(ctxt->md5_buf, ctxt);
for (i = gap; i + MD5_BUFLEN <= len; i += MD5_BUFLEN)
- md5_calc((uint8 *) (input + i), ctxt);
+ md5_calc(unconstify(uint8 *, (input + i)), ctxt);
ctxt->md5_i = len - i;
memmove(ctxt->md5_buf, input + i, ctxt->md5_i);
diff --git a/contrib/pgcrypto/pgp-compress.c b/cont
