Horton

Thread

Commits

  1. Get rid of another unconstify through API changes

  2. Resolve one unconstify use

  3. More unconstify use

  4. Remove useless casts

  1. more unconstify use

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-07T08:14:34Z

    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

  2. Re: more unconstify use

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-13T11:19:18Z

    On 07/02/2019 09:14, Peter Eisentraut 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.

committed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: more unconstify use

    Mark Dilger <hornschnorter@gmail.com> — 2019-02-13T18:51:32Z

    > 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

  4. Re: more unconstify use

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-14T20:07:45Z

    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