{"thread":{"id":"21845","subject":"[RFC PATCH 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","startedAt":"2009-12-03T18:44:11Z","lastAt":"2009-12-04T22:58:13Z","messageCount":5,"participants":["Ramsay Jones","Johannes Sixt","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"129132","messageId":"4B1806FB.2050401@ramsay1.demon.co.uk","threadId":"21845","inReplyTo":null,"subject":"[RFC PATCH 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-12-03T18:44:11Z","receivedAt":"2009-12-03T18:44:11Z","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 use the appropriate\nstructure type names (and function names) from the msvc headers.\nThis allows us to compile with -D_USE_32BIT_TIME_T if necessary.\nNote that the original mingw code should work with an msvc/sdk\nprior to 2005. We attempt to detect this by checking for _stati64\nbeing defined as a macro and, if not defined, conditionally\ncompiling the original code.\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nThe first version of this patch was much simpler; the diffstat showed\na net decrease of 15 lines of code! The extra fat comes from additions\nto the compat/mingw.h file. The original change looked like this (along\nwith the identical change to compat/msvc.h):\n\n    @@ -175,13 +175,21 @@ 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    +#if defined(_MSC_VER)\n    +#define stat _stat64\n    +#else\n    +#define stat _stati64\n    +#endif\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    +#if defined(_MSC_VER)\n    +#define _stat64(x,y) mingw_lstat(x,y)\n    +#else\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\n\nThis works with my version of msvc/sdk, provided we have no need to compile\nwith -D_USE_32BIT_TIME_T. (I was a little concerned when I noticed that the\ntime_t type was 64-bits; I checked a few of the obvious places to see if this\ncauses any breakage, but didn't find any).\n\nAlso, I added the \"&& defined(_stati64)\" in the hope that it would work with\nolder msvc/sdk versions.\n\nThe reason for the RFC is:\n\n    - maybe we don't need the flexibility of compiling with/without the 32-bit\n      time_t definition (which *works* BTW) and can revert to the original patch?\n    - I *think* this will work with older msvc, but I can't test it!\n    - I've tried to be careful not to break the MinGW build, but again I can't\n      test it. (I will be shocked if I have ;-)\n\nATB,\nRamsay Jones\n\n compat/mingw.h |   27 ++++++++++++++++++++++++++-\n compat/msvc.h  |   25 +------------------------\n 2 files changed, 27 insertions(+), 25 deletions(-)\n\ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex 5b5258b..98d233b 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -175,14 +175,39 @@ 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+\n+#if defined(_MSC_VER) && defined(_stati64)\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+#else  /* !defined(_MSC_VER) || !defined(_stati64) */\n+\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 \n+#endif\n+\n int mingw_utime(const char *file_name, const struct utimbuf *times);\n #define utime mingw_utime\n \ndiff --git a/compat/msvc.h b/compat/msvc.h\nindex 9c753a5..c099fe0 100644\n--- a/compat/msvc.h\n+++ b/compat/msvc.h\n@@ -21,30 +21,7 @@ static __inline int strcasecmp (const char *s1, const char *s2)\n }\n \n #undef ERROR\n-#undef stat\n-#undef _stati64\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 #endif\n-- \n1.6.5\n"},{"id":"129166","messageId":"4B18BDE7.3050004@viscovery.net","threadId":"21845","inReplyTo":"4B1806FB.2050401@ramsay1.demon.co.uk","subject":"Re: [RFC PATCH 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-12-04T07:44:39Z","receivedAt":"2009-12-04T07:44:39Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ramsay Jones schrieb:\n> In order to avoid the compiler warning, we use the appropriate\n> structure type names (and function names) from the msvc headers.\n> This allows us to compile with -D_USE_32BIT_TIME_T if necessary.\n\n\"if necessary\"? Who defines when -D_USE_32BIT_TIME_T is necessary?\n\n> Also, I added the \"&& defined(_stati64)\" in the hope that it would work with\n> older msvc/sdk versions.\n\nI think that this is an unnecessary complication and I did wonder why you\nadded this extra check. Anybody doing some serious development with MS's\ntools is using VS2005 at least. But isn't the .vcproj file made for VS2008\nanyway?\n\n> The reason for the RFC is:\n> \n>     - maybe we don't need the flexibility of compiling with/without the 32-bit\n>       time_t definition (which *works* BTW) and can revert to the original patch?\n\nIndeed I'm wondering why we should cater for 64 bit time_t. It is an\nunnessary complication as long as MinGW gcc supports only 32 bit time_t\nand the old MSVCRT.DLL.\n\n>     - I *think* this will work with older msvc, but I can't test it!\n\nThis should not be a concern, IMHO.\n\n>     - I've tried to be careful not to break the MinGW build, but again I can't\n>       test it. (I will be shocked if I have ;-)\n\nIt compiles without warnings and doesn't break t/t[01]* ;)\n\n-- Hannes\n"},{"id":"129172","messageId":"alpine.DEB.1.00.0912041144470.4985@pacific.mpi-cbg.de","threadId":"21845","inReplyTo":"4B1806FB.2050401@ramsay1.demon.co.uk","subject":"Re: [RFC PATCH 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-12-04T10:47:17Z","receivedAt":"2009-12-04T10:47:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 3 Dec 2009, Ramsay Jones wrote:\n\n>  compat/mingw.h |   27 ++++++++++++++++++++++++++-\n>  compat/msvc.h  |   25 +------------------------\n>  2 files changed, 27 insertions(+), 25 deletions(-)\n\nI'd prefer to have the MSVC-specific definitions in msvc.h, along with a \ndefinition of, say, ALREADY_DEFINED_STATI64 or some such (which tells \nmingw.h not to do anything about those types).  There is no need to \nclutter mingw.h with stuff for MSVC.\n\nCiao,\nDscho\n"},{"id":"129251","messageId":"4B199239.2050709@ramsay1.demon.co.uk","threadId":"21845","inReplyTo":"4B18BDE7.3050004@viscovery.net","subject":"Re: [RFC PATCH 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-12-04T22:50:33Z","receivedAt":"2009-12-04T22:50:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Sixt wrote:\n> Ramsay Jones schrieb:\n>> In order to avoid the compiler warning, we use the appropriate\n>> structure type names (and function names) from the msvc headers.\n>> This allows us to compile with -D_USE_32BIT_TIME_T if necessary.\n> \n> \"if necessary\"? Who defines when -D_USE_32BIT_TIME_T is necessary?\n\nIf I'm reading the msdn documentation correctly, from msvc 2005 onwards,\nthe time_t type is 64-bits by default. In order to support \"legacy apps\"\nwhich break with a 64-bit time_t, the _USE_32BIT_TIME_T macro may be set\nin the makefile/project-file to enable the old 32-bit time_t type.\n(The headers contain a lot of appropriately defined \"static __inline\"\nfunctions which call 32- or 64-bit versions of the main time related\nfunctions; eg. mktime() will call either _mktime64() or _mktime32()).\n\nNote that 64-bit windows does not support a 32-bit time_t.\n\nSo, the \"if necessary\" means: if git is broken with a 64-bit time_t and it\nmay take some time to fix it (or we can defer fixing it for a long time).\nAnd the \"Who\" is: er... well \"us\" ;-)\n\n>> Also, I added the \"&& defined(_stati64)\" in the hope that it would work with\n>> older msvc/sdk versions.\n> \n> I think that this is an unnecessary complication and I did wonder why you\n> added this extra check. Anybody doing some serious development with MS's\n> tools is using VS2005 at least.\n\nGreat! So, I will drop that part of the patch.\n\n>                                  But isn't the .vcproj file made for VS2008\n> anyway?\n\nI don't know - I don't use it. (Marius?)\n\n>> The reason for the RFC is:\n>>\n>>     - maybe we don't need the flexibility of compiling with/without the 32-bit\n>>       time_t definition (which *works* BTW) and can revert to the original patch?\n> \n> Indeed I'm wondering why we should cater for 64 bit time_t. It is an\n> unnessary complication as long as MinGW gcc supports only 32 bit time_t\n> and the old MSVCRT.DLL.\n\nAh, so you want to add -D_USE_32BIT_TIME_T to the Makefile?\nDo we care about 64-bit Windows?\n\n> \n>>     - I've tried to be careful not to break the MinGW build, but again I can't\n>>       test it. (I will be shocked if I have ;-)\n> \n> It compiles without warnings and doesn't break t/t[01]* ;)\n\nThanks! v2 of patch comming soon.\n\nATB,\nRamsay Jones\n"},{"id":"129252","messageId":"4B199405.8040607@ramsay1.demon.co.uk","threadId":"21845","inReplyTo":"alpine.DEB.1.00.0912041144470.4985@pacific.mpi-cbg.de","subject":"Re: [RFC PATCH 2/2] MSVC: Fix an \"incompatible pointer types\" compiler warning","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-12-04T22:58:13Z","receivedAt":"2009-12-04T22:58:13Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Johannes Schindelin wrote:\n> On Thu, 3 Dec 2009, Ramsay Jones wrote:\n> \n>>  compat/mingw.h |   27 ++++++++++++++++++++++++++-\n>>  compat/msvc.h  |   25 +------------------------\n>>  2 files changed, 27 insertions(+), 25 deletions(-)\n> \n> I'd prefer to have the MSVC-specific definitions in msvc.h, along with a \n> definition of, say, ALREADY_DEFINED_STATI64 or some such (which tells \n> mingw.h not to do anything about those types).  There is no need to \n> clutter mingw.h with stuff for MSVC.\n\nAh, yeah, I did think about this. As I said, the original patch was much\nsimpler and I thought the change to mingw.h was just this side of being\nobjectionable ;-) However, as the patch continued to gain weight I should\nhave re-evaluated that... my bad.\n\nNew version of the patch coming soon.\n\nATB,\nRamsay Jones\n"}]}