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,
> &output, &outputlen,
> 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