On 04.11.26 03:46, Chao Li wrote:
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.
```
Hmm, I see what you mean, but I think this would distract from the point
of the comment, which is about the pointer type, not about whether it's
qualified.
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);
```
Well, we could, but then we should justify why this is safe. The way I
have written it, we don't have to do that but we can instead let the
code speak for itself.
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.
No, VARDATA_EXTERNAL is different, because it really should be
const-preserving, like in C++ you would overload it like
static inline char *VARDATA_EXTERNAL(void *PTR);
static inline const char *VARDATA_EXTERNAL(const void *PTR);
lnext() is not const-preserving. The input cell is always const and the
output cell is always non-const. The old code was just using the "c"
variable for both, which was slightly wrong.
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.
You can't change BuildTupleFromCStrings(), because then you would break
callers who want to pass in a char **. You could make a separate new
function that takes a const char **. But it doesn't seem worth it for
just one caller.
4 - After applying the patch set, my local build still gets a couple of
warnings:
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';
```
Ok, what platform and libreadline/libedit version is this? (It's
apparently not covered on CI.)
But that doesn’t feel good, because we are writing to the pointers, forcing to
unconstify those points might be unsafe.
Right, but we are already doing it. Probably the underlying storage of
what those pointers point to is not const, so it would be ok.
