Re: more unconstify use - Mailing list pgsql-hackers

> On Feb 7, 2019, at 12:14 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
> <v1-0001-Remove-useless-casts.patch><v1-0002-More-unconstify-use.patch>

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-184E11DE68F3@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,
                                       &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.



mark