{"thread":{"id":"21867","subject":"[RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","startedAt":"2009-12-04T23:13:36Z","lastAt":"2009-12-10T18:38:59Z","messageCount":6,"participants":["Ramsay Jones","Johannes Sixt","Johannes Schindelin"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"129253","messageId":"4B1997A0.9000004@ramsay1.demon.co.uk","threadId":"21867","inReplyTo":null,"subject":"[RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-12-04T23:13:36Z","receivedAt":"2009-12-04T23:13:36Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nIn particular, the following warning is issued while compiling\ncompat/msvc.c:\n\n    ...mingw.c(223) : warning C4133: 'function' : incompatible \\\ntypes - from '_stati64 *' to '_stat64 *'\n\nwhich relates to a call of _fstati64() in the mingw_fstat()\nfunction definition.\n\nThis is caused by various layers of macro magic and attempts to\navoid macro redefinition compiler warnings. For example, the call\nto _fstati64() mentioned above is actually a call to _fstat64(),\nsince macro _USE_32BIT_TIME_T is not defined, and expects a pointer\nto a struct _stat64 rather than the struct _stati64 which is passed\nto mingw_fstat().\n\nThe definition of struct _stati64 given in compat/msvc.h had the\nsame \"shape\" as the definition of struct _stat64, so the call to\n_fstat64() does not actually cause any runtime errors, but the\nstructure types are indeed incompatible. Also, the \"shape\" of\nstruct _stati64 changes, depending on the _USE_32BIT_TIME_T\nmacro, since the time_t type is defined as either __time64_t or\n__time32_t.\n\nWhen _USE_32BIT_TIME_T is defined, the call to _fstati64() is\nactually a call to _fstat32i64() and expects a struct _stat32i64\npointer parameter. (struct _stati64 would again have the same\n\"shape\" as struct _stat32i64).\n\nThe _USE_32BIT_TIME_T macro, along with all of the additional\nstructure type definitions, function definitions, and overloading\nmacro magic was introduced in msvc 2005.\n\nIn order to avoid the compiler warning, we add declarations for the\nmingw_lstat() and mingw_fstat() functions and supporting macros to\nmsvc.h, suppressing the corresponding declarations in mingw.h, so\nthat we can use the appropriate structure type (and function) names\nfrom the msvc headers.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nChanges from v1:\n    - moved the new declarations to msvc.h rather than clutter mingw.h\n      with msvc related code.\n    - don't even attempt to support older msvc compilers\n\nThe patch is still marked RFC because:\n    - I'm still not sure if the flexibility to support both 32- and 64-bit\n      time_t is required.\n    - should -D_USE_32BIT_TIME_T be added to the Makefile?\n\nATB,\nRamsay Jones\n\n compat/mingw.h |    4 +++-\n compat/msvc.h  |   54 +++++++++++++++++++++++++++++-------------------------\n 2 files changed, 32 insertions(+), 26 deletions(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 5b5258b..b5cec7f 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -175,13 +175,15 @@ int mingw_getpagesize(void);\n  * mingw_fstat() instead of fstat() on Windows.\n  */\n #define off_t off64_t\n-#define stat _stati64\n #define lseek _lseeki64\n+#ifndef ALREADY_DECLARED_STAT_FUNCS\n+#define stat _stati64\n int mingw_lstat(const char *file_name, struct stat *buf);\n int mingw_fstat(int fd, struct stat *buf);\n #define fstat mingw_fstat\n #define lstat mingw_lstat\n #define _stati64(x,y) mingw_lstat(x,y)\n+#endif\n \n int mingw_utime(const char *file_name, const struct utimbuf *times);\n #define utime mingw_utime\ndiff --git a/compat/msvc.h b/compat/msvc.h\nindex 9c753a5..f431007 100644\n--- a/compat/msvc.h\n+++ b/compat/msvc.h\n@@ -21,30 +21,34 @@ static __inline int strcasecmp (const char *s1, const char *s2)\n }\n \n #undef ERROR\n-#undef stat\n-#undef _stati64\n+\n+/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead\n+ * of fstat(). We add the declaration of these functions here, suppressing\n+ * the corresponding declarations in mingw.h, so that we can use the\n+ * appropriate structure type (and function) names from the msvc headers.\n+ */\n+#if defined(_USE_32BIT_TIME_T)\n+# define stat _stat32i64\n+#else\n+# define stat _stat64\n+#endif\n+\n+int mingw_lstat(const char *file_name, struct stat *buf);\n+int mingw_fstat(int fd, struct stat *buf);\n+\n+#define fstat mingw_fstat\n+#define lstat mingw_lstat\n+\n+#if defined(_USE_32BIT_TIME_T)\n+# define _stat32i64(x,y) mingw_lstat(x,y)\n+#else\n+# define _stat64(x,y) mingw_lstat(x,y)\n+#endif\n+\n+#define ALREADY_DECLARED_STAT_FUNCS\n+\n #include \"compat/mingw.h\"\n-#undef stat\n-#define stat _stati64\n-#define _stat64(x,y) mingw_lstat(x,y)\n-\n-/*\n-   Even though _stati64 is normally just defined at _stat64\n-   on Windows, we specify it here as a proper struct to avoid\n-   compiler warnings about macro redefinition due to magic in\n-   mingw.h. Struct taken from ReactOS (GNU GPL license).\n-*/\n-struct _stati64 {\n-\t_dev_t  st_dev;\n-\t_ino_t  st_ino;\n-\tunsigned short st_mode;\n-\tshort   st_nlink;\n-\tshort   st_uid;\n-\tshort   st_gid;\n-\t_dev_t  st_rdev;\n-\t__int64 st_size;\n-\ttime_t  st_atime;\n-\ttime_t  st_mtime;\n-\ttime_t  st_ctime;\n-};\n+\n+#undef ALREADY_DECLARED_STAT_FUNCS\n+\n #endif\n-- \n1.6.5\n"},{"id":"129262","messageId":"200912051257.21386.j6t@kdbg.org","threadId":"21867","inReplyTo":"4B1997A0.9000004@ramsay1.demon.co.uk","subject":"Re: [RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-05T11:57:19Z","receivedAt":"2009-12-05T11:57:19Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"[Removed Junio from Cc because IIUC, he prefers to be left out of the loop in \nWindows related matters until they have settled.]\n\nOn Samstag, 5. Dezember 2009, Ramsay Jones wrote:\n> The patch is still marked RFC because:\n>     - I'm still not sure if the flexibility to support both 32- and 64-bit\n>       time_t is required.\n>     - should -D_USE_32BIT_TIME_T be added to the Makefile?\n\nIf *not* using -D_USE_32BIT_TIME_T  produces a build or code base that is in \nsome way superior, why should we require it? For example, its absence could \nhelp a 64bit build.\n\n-- Hannes\n"},{"id":"129268","messageId":"alpine.DEB.1.00.0912051556290.4985@pacific.mpi-cbg.de","threadId":"21867","inReplyTo":"4B1997A0.9000004@ramsay1.demon.co.uk","subject":"Re: [RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-12-05T14:57:03Z","receivedAt":"2009-12-05T14:57:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 4 Dec 2009, Ramsay Jones wrote:\n\n> Changes from v1:\n>     - moved the new declarations to msvc.h rather than clutter mingw.h\n>       with msvc related code.\n\nThanks,\nDscho\n"},{"id":"129569","messageId":"4B1EAD9A.3090205@ramsay1.demon.co.uk","threadId":"21867","inReplyTo":"200912051257.21386.j6t@kdbg.org","subject":"Re: [RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-12-08T19:48:42Z","receivedAt":"2009-12-08T19:48:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> On Samstag, 5. Dezember 2009, Ramsay Jones wrote:\n>> The patch is still marked RFC because:\n>>     - I'm still not sure if the flexibility to support both 32- and 64-bit\n>>       time_t is required.\n>>     - should -D_USE_32BIT_TIME_T be added to the Makefile?\n> \n> If *not* using -D_USE_32BIT_TIME_T  produces a build or code base that is in \n> some way superior, why should we require it? For example, its absence could \n> help a 64bit build.\n\nIndeed.\n\nI only added the second bullet because you seemed to be advocating it, in your\nlast email, so I'm waiting for your answer on that. :-D\n\nUnfortunately the patch can not be a simple as the first version (Dscho was\nquite right to complain about adding too much clutter to mingw.h with msvc\nrelated code), but the moral equivalent would have msvc.h look like:\n\n    25\t/* Use mingw_lstat() instead of lstat()/stat() and mingw_fstat() instead\n    26\t * of fstat(). We add the declaration of these functions here, suppressing\n    27\t * the corresponding declarations in mingw.h, so that we can use the\n    28\t * appropriate structure type (and function) names from the msvc headers.\n    29\t */\n    30\t#define stat _stat64\n    31\tint mingw_lstat(const char *file_name, struct stat *buf);\n    32\tint mingw_fstat(int fd, struct stat *buf);\n    33\t#define fstat mingw_fstat\n    34\t#define lstat mingw_lstat\n    35\t#define _stat64(x,y) mingw_lstat(x,y)\n    36\t#define ALREADY_DECLARED_STAT_FUNCS\n    37\t\n    38\t#include \"compat/mingw.h\"\n    39\t\n    40\t#undef ALREADY_DECLARED_STAT_FUNCS\n\nThis works fine, *provided* you do not need to compile with -D_USE_32BIT_TIME_T,\nwhich would produce this warning:\n\n    ...mingw.c(223) : warning C4133: 'function' : incompatible types - \\\n    from '_stat64 *' to '_stat32i64 *'\n\nThis would actually be *worse* than the original code, since the struct _stat64\nwould not have the same \"shape\" as the struct _stat32i64; it would not write\noutside of the allocated memory, at least, but the time fields would obviously\nbe written to the wrong offsets etc,. In the original code, the struct _stati64\nwould have the correct \"shape\", since the time fields are declared with time_t.\n\nAt first I thought there would be no need to set _USE_32BIT_TIME_T.  After some\nthought, however, I could not be confident that it would *never* be necessary.\n(my only concern was to revert to 32-bit time_t, perhaps only temporarily, while\nfixing a breakage; I did not consider wanting to keep \"compatibility\" with the\nMinGW code). This lead to the more complicated/flexible RFC patch.\n\nI was hoping for someone to say: \"Hey, we will *never* need to compile with\n-D_USE_32BIT_TIME_T, so just get rid of those #if conditionals and simplify\nthe code ...\". :-P\n\nSince nobody has said any such thing, I have to conclude that the extra\nflexibility is required. That being the case, I have to be happy with the\npatch as-is and propose to remove the RFC.\n\nBefore I do that, do you have any further comments or concerns about the\nv2 patch that you want me to address?\n\nATB,\nRamsay Jones\n"},{"id":"129574","messageId":"200912082131.44498.j6t@kdbg.org","threadId":"21867","inReplyTo":"4B1EAD9A.3090205@ramsay1.demon.co.uk","subject":"Re: [RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2009-12-08T20:31:43Z","receivedAt":"2009-12-08T20:31:43Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"On Dienstag, 8. Dezember 2009, Ramsay Jones wrote:\n>     30\t#define stat _stat64\n>     31\tint mingw_lstat(const char *file_name, struct stat *buf);\n>     32\tint mingw_fstat(int fd, struct stat *buf);\n>     33\t#define fstat mingw_fstat\n>     34\t#define lstat mingw_lstat\n>     35\t#define _stat64(x,y) mingw_lstat(x,y)\n>     36\t#define ALREADY_DECLARED_STAT_FUNCS\n>     37\n>     38\t#include \"compat/mingw.h\"\n>     39\n>     40\t#undef ALREADY_DECLARED_STAT_FUNCS\n>\n> This works fine, *provided* you do not need to compile with\n> -D_USE_32BIT_TIME_T, which would produce this warning:\n>\n>     ...mingw.c(223) : warning C4133: 'function' : incompatible types - \\\n>     from '_stat64 *' to '_stat32i64 *'\n>\n> This would actually be *worse* than the original code, since the struct\n> _stat64 would not have the same \"shape\" as the struct _stat32i64; ...\n\nTo cut this short: According to your explanations, using -D_USE_32BIT_TIME_T \nwith MSVC is bad. Please reroll without references to _USE_32BIT_TIME_T.\n\n-- Hannes\n"},{"id":"129702","messageId":"4B214043.8060900@ramsay1.demon.co.uk","threadId":"21867","inReplyTo":"200912082131.44498.j6t@kdbg.org","subject":"Re: [RFC PATCH v2 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-12-10T18:38:59Z","receivedAt":"2009-12-10T18:38:59Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> To cut this short: According to your explanations, using -D_USE_32BIT_TIME_T \n> with MSVC is bad. Please reroll without references to _USE_32BIT_TIME_T.\n\nOK, will do...\n\nATB,\nRamsay Jones\n"}]}