Thread: Cast to uint16 in pg_checksum_page()
Hackers,
The current code in checksum_impl.h does not play nice with -Wconversion
on gcc:
warning: conversion to 'uint16 {aka short unsigned int}' from 'uint32
{aka unsigned int}' may alter its value [-Wconversion]
return (checksum % 65535) + 1;
~~~~~~~~~~~~~~~~~~~^~~
It seems like an explicit cast to uint16 would be better?
Regards,
--
-David
david@pgmasters.net
Attachment
On Tue, Mar 03, 2020 at 06:37:36PM -0500, David Steele wrote:
> Hackers,
>
> The current code in checksum_impl.h does not play nice with -Wconversion on
> gcc:
>
> warning: conversion to 'uint16 {aka short unsigned int}' from 'uint32 {aka
> unsigned int}' may alter its value [-Wconversion]
> return (checksum % 65535) + 1;
> ~~~~~~~~~~~~~~~~~~~^~~
>
> It seems like an explicit cast to uint16 would be better?
Attempting to compile the backend code with -Wconversion leads to many
warnings, still there has been at least one fix in the past to ease
the use of the headers in this case, with b5b3229 (this made the code
more readable). Should we really care about this case?
--
Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Mar 03, 2020 at 06:37:36PM -0500, David Steele wrote:
>> It seems like an explicit cast to uint16 would be better?
> Attempting to compile the backend code with -Wconversion leads to many
> warnings, still there has been at least one fix in the past to ease
> the use of the headers in this case, with b5b3229 (this made the code
> more readable). Should we really care about this case?
Per the commit message for b5b3229, it might be worth getting rid of
such messages for code that's exposed in header files, even if removing
all of those warnings would be too much work. Perhaps David's use-case
is an extension that's using that header?
regards, tom lane
On 3/4/20 1:05 AM, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: >> On Tue, Mar 03, 2020 at 06:37:36PM -0500, David Steele wrote: >>> It seems like an explicit cast to uint16 would be better? > >> Attempting to compile the backend code with -Wconversion leads to many >> warnings, still there has been at least one fix in the past to ease >> the use of the headers in this case, with b5b3229 (this made the code >> more readable). Should we really care about this case? > > Per the commit message for b5b3229, it might be worth getting rid of > such messages for code that's exposed in header files, even if removing > all of those warnings would be too much work. Perhaps David's use-case > is an extension that's using that header? Yes, this is being included in an external project. Previously we have used a highly marked-up version but we are now trying to pull in the header more or less verbatim. Since this header is specifically designated as something external projects may want to use I think it makes sense to fix the warning. Regards, -- -David david@pgmasters.net
On Wed, Mar 04, 2020 at 07:02:43AM -0500, David Steele wrote: > Yes, this is being included in an external project. Previously we have used > a highly marked-up version but we are now trying to pull in the header more > or less verbatim. > > Since this header is specifically designated as something external projects > may want to use I think it makes sense to fix the warning. This sounds like a sensible argument, similar to the ones raised on the other thread, so no objections from me to improve things here. I can look at that tomorrow, except if somebody else beats me to it. -- Michael
Attachment
On Wed, Mar 04, 2020 at 09:52:08PM +0900, Michael Paquier wrote: > This sounds like a sensible argument, similar to the ones raised on > the other thread, so no objections from me to improve things here. I > can look at that tomorrow, except if somebody else beats me to it. And done. -- Michael