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
