> On Nov 3, 2026, at 23:34, Peter Eisentraut <[email protected]> wrote:
>
> We have been progressively cleaning up -Wcast-qual warnings this year. This
> patch set is the final part of this, containing a few tricky, complicated,
> and/or ugly cases that I have been putting off. And then we can turn
> -Wcast-qual on by default. Each patch is individually described in its
> commit message. Alternative approaches are possible; feedback
> welcome.<0001-Fix-accidentally-casting-away-const.patch><0002-Fix-Wcast-qual-warning-in-lnext.patch><0003-Fix-Wcast-qual-warning-in-foreach_delete_current.patch><0004-Fix-Wcast-qual-warning-in-XLogInsertRecord.patch><0005-Fix-Wcast-qual-warnings-in-regex-module.patch><0006-Fix-Wcast-qual-warning-in-guc_funcs.c.patch><0007-Add-unconstify_pointee.patch><0008-Fix-Wcast-qual-warnings-in-read-write-accessor-macro.patch><0009-Adjust-unconstify-unvolatize-to-avoid-Wcast-qual-war.patch><0010-Add-warning-option-Wcast-qual.patch>
The patch set overall looks good to me. I have a few comments as below:
1 - 0001
```
--- a/src/include/access/detoast.h
+++ b/src/include/access/detoast.h
@@ -23,7 +23,7 @@
*/
#define VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr) \
do { \
- varattrib_1b_e *attre = (varattrib_1b_e *) (attr); \
+ const varattrib_1b_e *attre = (const varattrib_1b_e *) (attr); \
```
Looks like the header comment needs to be updated accordingly:
```
* intermediate "varattrib_1b_e *" variable seems to fix it.
```
2 - 0002
```
static inline ListCell *
lnext(const List *l, const ListCell *c)
{
- Assert(c >= &l->elements[0] && c < &l->elements[l->length]);
- c++;
- if (c < &l->elements[l->length])
- return (ListCell *) c;
+ int n = list_cell_number(l, c) + 1;
+
+ if (n < l->length)
+ return &l->elements[n];
else
return NULL;
}
```
Here, I think the problem is that c is const while the return value is not
const. Can't we just use unconstify(), like:
```
return unconstify(ListCell *, c);
```
Feels like VARDATA_EXTERNAL is similar use case that is used in 0001. It calls
VARDATA_EXTERNAL(attre), where attre is const, and VARDATA_EXTERNAL() uses
unconstify() to make it non-const.
3 - 0006
Since BuildTupleFromCStrings doesn’t update values, making values as a const
feels more reasonable, like:
```
extern HeapTuple BuildTupleFromCStrings(AttInMetadata *attinmeta, const char
*const*values);
```
I tried that, but that required more callers’ changes. So, I’d reserve this
comment, maybe we can do that with a separate refactoring patch.
4 - After applying the patch set, my local build still gets a couple of
warnings:
```
input.c:306:27: warning: cast from 'const char *' to 'char *' drops const
qualifier [-Wcast-qual]
306 | for (cur_ptr = (char *) cur_hist->line; *cur_ptr;
cur_ptr++)
| ^
input.c:326:27: warning: cast from 'const char *' to 'char *' drops const
qualifier [-Wcast-qual]
326 | for (cur_ptr = (char *) cur_hist->line; *cur_ptr;
cur_ptr++)
| ^
2 warnings generated.
```
We can fix them by using unconstify:
```
diff --git a/src/bin/psql/input.c b/src/bin/psql/input.c
index 6d37359fc96..177434ed975 100644
--- a/src/bin/psql/input.c
+++ b/src/bin/psql/input.c
@@ -303,7 +303,7 @@ encode_history(void)
char *cur_ptr;
/* some platforms declare HIST_ENTRY.line as const char * */
- for (cur_ptr = (char *) cur_hist->line; *cur_ptr; cur_ptr++)
+ for (cur_ptr = unconstify(char *, cur_hist->line); *cur_ptr;
cur_ptr++)
{
if (*cur_ptr == '\n')
*cur_ptr = NL_IN_HISTORY;
@@ -323,7 +323,7 @@ decode_history(void)
char *cur_ptr;
/* some platforms declare HIST_ENTRY.line as const char * */
- for (cur_ptr = (char *) cur_hist->line; *cur_ptr; cur_ptr++)
+ for (cur_ptr = unconstify(char *, cur_hist->line); *cur_ptr;
cur_ptr++)
{
if (*cur_ptr == NL_IN_HISTORY)
*cur_ptr = '\n';
```
But that doesn’t feel good, because we are writing to the pointers, forcing to
unconstify those points might be unsafe.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
