Hi David, Thank you for reminding about this patch!
Here is a new patch. I tried to make as little changes as possible. This is no doubt not the most beautiful patch on Earth but it removes all warnings. I anyone could suggest an approach that would be significantly better please don't hesitate to share your ideas. Tested on Clang 3.9.1 and GCC 6.3.1. > Why not update the target type to "unsigned char" instead, so that no > cast is needed and the value compatibility is checked by the compiler? I > guess there would be some more changes (question is how much), but it > would be cleaner. I tried this way as well. After rebuilding PostgreSQL in 5th time I discovered that now I have to redefine a Poiner typedef. I don't think we can avoid using type casts. There will be just different type casts in other places, or we'll have to break most of existing PostgreSQL extensions. On Mon, Mar 13, 2017 at 10:19:57AM -0400, David Steele wrote: > Hi Aleksander, > > On 2/22/17 9:43 AM, Fabien COELHO wrote: > > > > Hello Aleksander, > > > >> ``` > >> xloginsert.c:742:18: warning: implicit conversion from 'int' to 'char' > >> changes value from 253 to -3 [-Wconstant-conversion] > >> ``` > > > > There is a bunch of these in "xlog.c" as well, and the same code is used > > in "pg_resetwal.c". > > > >> Patch that fixes these warnings is attached to this email. > > > > My 0.02€: > > > > I'm not at ease at putting the thing bluntly under the carpet with a cast. > > > > Why not update the target type to "unsigned char" instead, so that no > > cast is needed and the value compatibility is checked by the compiler? I > > guess there would be some more changes (question is how much), but it > > would be cleaner. > > There's been no discussion or new patch on this thread recently. If you > are planning to address the issues raised please plan to do so by > Thursday, March 16th. > > If no new patch is submitted by that date I will mark this patch > "Returned with Feedback". > > Thanks, > -- > -David > da...@pgmasters.net -- Best regards, Aleksander Alekseev
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 744360c769..6aa93b5ff7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -5018,7 +5018,7 @@ BootStrapXLOG(void)
record->xl_rmid = RM_XLOG_ID;
recptr += SizeOfXLogRecord;
/* fill the XLogRecordDataHeaderShort struct */
- *(recptr++) = XLR_BLOCK_ID_DATA_SHORT;
+ *(recptr++) = (char)XLR_BLOCK_ID_DATA_SHORT;
*(recptr++) = sizeof(checkPoint);
memcpy(recptr, &checkPoint, sizeof(checkPoint));
recptr += sizeof(checkPoint);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 03b05f937f..ea8e915029 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -739,7 +739,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
if ((curinsert_flags & XLOG_INCLUDE_ORIGIN) &&
replorigin_session_origin != InvalidRepOriginId)
{
- *(scratch++) = XLR_BLOCK_ID_ORIGIN;
+ *(scratch++) = (char)XLR_BLOCK_ID_ORIGIN;
memcpy(scratch, &replorigin_session_origin, sizeof(replorigin_session_origin));
scratch += sizeof(replorigin_session_origin);
}
@@ -749,13 +749,13 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
{
if (mainrdata_len > 255)
{
- *(scratch++) = XLR_BLOCK_ID_DATA_LONG;
+ *(scratch++) = (char)XLR_BLOCK_ID_DATA_LONG;
memcpy(scratch, &mainrdata_len, sizeof(uint32));
scratch += sizeof(uint32);
}
else
{
- *(scratch++) = XLR_BLOCK_ID_DATA_SHORT;
+ *(scratch++) = (char)XLR_BLOCK_ID_DATA_SHORT;
*(scratch++) = (uint8) mainrdata_len;
}
rdt_datas_last->next = mainrdata_head;
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index 27bd9b04e7..ed6968d605 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -1095,7 +1095,7 @@ WriteEmptyXLOG(void)
record->xl_rmid = RM_XLOG_ID;
recptr += SizeOfXLogRecord;
- *(recptr++) = XLR_BLOCK_ID_DATA_SHORT;
+ *(recptr++) = (char)XLR_BLOCK_ID_DATA_SHORT;
*(recptr++) = sizeof(CheckPoint);
memcpy(recptr, &ControlFile.checkPointCopy,
sizeof(CheckPoint));
diff --git a/src/include/port.h b/src/include/port.h
index f4546016e7..0f014b72b6 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -395,11 +395,22 @@ extern double rint(double x);
extern int inet_aton(const char *cp, struct in_addr * addr);
#endif
-#if !HAVE_DECL_STRLCAT
+/*
+ * Unfortunately in case of strlcat and strlcpy we can't trust tests
+ * executed by Autotools if Clang > 3.6 is used. Clang manages to compile
+ * a program that shouldn't compile which causes wrong values of
+ * HAVE_DECL_STRLCAT and HAVE_DECL_STRLCPY. More details could be found here:
+ *
+ * http://lists.llvm.org/pipermail/cfe-dev/2016-March/048126.html
+ *
+ * This is no doubt a dirty hack but apparently alternative solutions are
+ * not much better.
+ */
+#if !HAVE_DECL_STRLCAT || defined(__clang__)
extern size_t strlcat(char *dst, const char *src, size_t siz);
#endif
-#if !HAVE_DECL_STRLCPY
+#if !HAVE_DECL_STRLCPY || defined(__clang__)
extern size_t strlcpy(char *dst, const char *src, size_t siz);
#endif
signature.asc
Description: PGP signature
