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
Attachments:
page-checksum-cast-v1.patchtext/plain; charset=UTF-8; name=page-checksum-cast-v1.patch; x-mac-creator=0; x-mac-type=0Download
diff --git a/src/include/storage/checksum_impl.h b/src/include/storage/checksum_impl.h
index 0f264dfe4c..b5a3e49e95 100644
--- a/src/include/storage/checksum_impl.h
+++ b/src/include/storage/checksum_impl.h
@@ -211,5 +211,5 @@ pg_checksum_page(char *page, BlockNumber blkno)
* Reduce to a uint16 (to fit in the pd_checksum field) with an offset of
* one. That avoids checksums of zero, which seems like a good idea.
*/
- return (checksum % 65535) + 1;
+ return (uint16)((checksum % 65535) + 1);
}
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
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
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