{"thread":{"id":"21522","subject":"[PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion","startedAt":"2009-11-07T20:22:16Z","lastAt":"2009-11-10T18:48:12Z","messageCount":5,"participants":["Ramsay Jones","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"127055","messageId":"4AF5D6F8.40608@ramsay1.demon.co.uk","threadId":"21522","inReplyTo":null,"subject":"[PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-11-07T20:22:16Z","receivedAt":"2009-11-07T20:22:16Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSince commit 435bdf8c (\"Make usage of windows.h lean and mean\",\n16-9-2009), the amount of code potentially including the WIN32\nAPI header files has greatly increased. In particular, the Cygwin\nbuild is at greater risk of inadvertently including WIN32 code\nwithin preprocessor sections protected by the WIN32 or _WIN32\nmacros.\n\nThe previous commit message, along with comments elsewhere, assert\nthat the WIN32 macro is not defined on Cygwin. Currently, this is\ntrue for the cygwin build. However, the cygwin platform can be\nused to develop WIN32 GUI, WIN32 console, and POSIX applications.\nIndeed it is possible to create applications which use a mix of\nthe WIN32 API and POSIX code (eg git!).\n\nUnlike native WIN32 compilers, gcc on cygwin does not automatically\ndefine the _WIN32 macro. However, as soon as you include the\n header file, the _WIN32 and WIN32 macros are defined.\n\nIn order to reduce the risk of problems in the future, we protect\nthe inclusion of the windows header with an explicit check for\n__CYGWIN__. Also, we move the other use of the header\nfrom compat/win32.h to compat/cygwin.c\n\nSigned-off-by: Ramsay Jones \n---\n\nChanges since v1:\n - removed the definition of the WIN32_API macro.\n - removed all usage of WIN32_API as a replacement for the use\n of the _WIN32 and WIN32 macros in #if(n)def sections.\n\nI wanted to separate these (remaining) changes from the WIN32_API\nissue, since that is likely to require further discussion (along\nwith some bike-shedding), and I didn't want these to get held up.\n\n compat/cygwin.c | 1 +\n compat/win32.h | 3 ---\n git-compat-util.h | 8 ++++----\n 3 files changed, 5 insertions(+), 7 deletions(-)\n\ndiff --git a/compat/cygwin.c b/compat/cygwin.c\nindex b4a51b9..6695515 100644\n--- a/compat/cygwin.c\n+++ b/compat/cygwin.c\n@@ -1,5 +1,6 @@\n #define WIN32_LEAN_AND_MEAN\n #include \"../git-compat-util.h\"\n+#include \n #include \"win32.h\"\n #include \"../cache.h\" /* to read configuration */\n \ndiff --git a/compat/win32.h b/compat/win32.h\nindex 8ce9104..a7ed72b 100644\n--- a/compat/win32.h\n+++ b/compat/win32.h\n@@ -2,9 +2,6 @@\n #define WIN32_H\n \n /* common Win32 functions for MinGW and Cygwin */\n-#ifndef WIN32 /* Not defined by Cygwin */\n-#include \n-#endif\n \n static inline int file_attr_to_st_mode (DWORD attr)\n {\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ef60803..c4b9e5a 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -65,10 +65,10 @@\n #define _NETBSD_SOURCE 1\n #define _SGI_SOURCE 1\n \n-#ifdef WIN32 /* Both MinGW and MSVC */\n-#define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n-#include \n-#include \n+#if defined(_WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */\n+# define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n+# include \n+# include \n #endif\n \n #include \n-- \n1.6.5\n"},{"id":"127123","messageId":"4AF7C2C6.5070307@viscovery.net","threadId":"21522","inReplyTo":"4AF5D6F8.40608@ramsay1.demon.co.uk","subject":"Re: [PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-11-09T07:20:38Z","receivedAt":"2009-11-09T07:20:38Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Ramsay Jones schrieb:\n> Since commit 435bdf8c (\"Make usage of windows.h lean and mean\",\n> 16-9-2009), the amount of code potentially including the WIN32\n> API header files has greatly increased. In particular, the Cygwin\n> build is at greater risk of inadvertently including WIN32 code\n> within preprocessor sections protected by the WIN32 or _WIN32\n> macros.\n\nThanks, this makes the problem pretty clear that you want to solve.\n\n> The previous commit message, along with comments elsewhere, assert\n> that the WIN32 macro is not defined on Cygwin. Currently, this is\n> true for the cygwin build. However, the cygwin platform can be\n> used to develop WIN32 GUI, WIN32 console, and POSIX applications.\n> Indeed it is possible to create applications which use a mix of\n> the WIN32 API and POSIX code (eg git!).\n\nIn this paragraph, you are only saying that cygwin comes with headers and\nlibraries that can be used to write code using the Windows API in addition\nto the POSIX headers and libraries. (I'm just asking, not complaining;\nperhaps this could be stated differently.)\n\n> Unlike native WIN32 compilers, gcc on cygwin does not automatically\n> define the _WIN32 macro. However, as soon as you include the\n> header file, the _WIN32 and WIN32 macros are defined.\n> \n> In order to reduce the risk of problems in the future, we protect\n> the inclusion of the windows header with an explicit check for\n> __CYGWIN__. Also, we move the other use of the header\n> from compat/win32.h to compat/cygwin.c\n\nBut I sense a contradiction here. Above you are arguing that much more\nWIN32 code is included, but here you are saying that the explicit check\nfor __CYGWIN__ is just a safety measure to protect us from failures in\nfuture changes. Indeed, looking at the code it seems that this extra check\nis *currently* not necessary:\n\n- Cygwin does not define WIN32, hence, in the original code of this hunk,\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index ef60803..c4b9e5a 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -65,10 +65,10 @@\n> #define _NETBSD_SOURCE 1\n> #define _SGI_SOURCE 1\n> \n> -#ifdef WIN32 /* Both MinGW and MSVC */\n> -#define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n> -#include \n> -#include \n> +#if defined(_WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */\n> +# define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n> +# include \n> +# include \n> #endif\n> \n> #include \n\nwindows.h is not included in cygwin. (Nor is it in the changed version.)\n\n- The other files that include windows.h are compat/win32.h and nedmalloc.\nThe latter isn't used on cygwin.\n\n- win32.h is included only from cygwin.c, mingw.c, and msvc.c. Only the\nfirst one is used by cygwin, and since it is a .c file, pulling in\nwindows.h can only concern code in cygwin.c, but not code.\n\nIOW, I disagree with your analysis that a lot of code suffers from\nwindows.h pollution. What am I missing?\n\n-- Hannes\n"},{"id":"127189","messageId":"4AF86CDB.6030200@ramsay1.demon.co.uk","threadId":"21522","inReplyTo":"4AF5D6F8.40608@ramsay1.demon.co.uk","subject":"Re: [PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-11-09T19:26:19Z","receivedAt":"2009-11-09T19:26:19Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Ramsay Jones wrote:\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index ef60803..c4b9e5a 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -65,10 +65,10 @@\n> #define _NETBSD_SOURCE 1\n> #define _SGI_SOURCE 1\n> \n> -#ifdef WIN32 /* Both MinGW and MSVC */\n> -#define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n> -#include \n> -#include \n> +#if defined(_WIN32) && !defined(__CYGWIN__) /* Both MinGW and MSVC */\n> +# define WIN32_LEAN_AND_MEAN /* stops windows.h including winsock.h */\n> +# include \n> +# include \n\nArrgghhh.\n\nSorry, Junio, I meant to change this before sending it out. :(\n\nI had intended to remove the added indentation (which I don't even remember\ndoing!), since it obscures the \"real change\", which simply consists of the\nnew expression in the #if preprocessor line.\n\nI'll send a new version of this patch.\n\nATB,\nRamsay Jones\n"},{"id":"127198","messageId":"7vocnbsaoy.fsf@alter.siamese.dyndns.org","threadId":"21522","inReplyTo":"4AF86CDB.6030200@ramsay1.demon.co.uk","subject":"Re: [PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-11-09T20:30:21Z","receivedAt":"2009-11-09T20:30:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> Ramsay Jones wrote:\n>> diff --git a/git-compat-util.h b/git-compat-util.h\n>>...\n>\n> Arrgghhh.\n>\n> Sorry, Junio, I meant to change this before sending it out. :(\n>\n> I had intended to remove the added indentation (which I don't even remember\n> doing!), since it obscures the \"real change\", which simply consists of the\n> new expression in the #if preprocessor line.\n>\n> I'll send a new version of this patch.\n\nThanks for a notice.\n\nThe only reason I queue patches to 'pu' is to give them (slightly) wider\nexposure than merely being in the mailing list archive, and I do not\nexpect myself merging this series to 'next' before seeing Acks from\nWindows stakeholders, as I do not build anything on that platform myself.\n\nI saw a comment on substance not form of this patch from J6t, and I\nsuspect that it was a more important issue. You can de-indent while\nsending your updated patch to address that issue.\n\nThanks.\n"},{"id":"127263","messageId":"4AF9B56C.5000205@ramsay1.demon.co.uk","threadId":"21522","inReplyTo":"4AF7C2C6.5070307@viscovery.net","subject":"Re: [PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2009-11-10T18:48:12Z","receivedAt":"2009-11-10T18:48:12Z","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>> Since commit 435bdf8c (\"Make usage of windows.h lean and mean\",\n>> 16-9-2009), the amount of code potentially including the WIN32\n>> API header files has greatly increased. In particular, the Cygwin\n>> build is at greater risk of inadvertently including WIN32 code\n>> within preprocessor sections protected by the WIN32 or _WIN32\n>> macros.\n> \n> Thanks, this makes the problem pretty clear that you want to solve.\n> \n\nHmm, OK. Note the \"... potentially including ...\" above.\n\n>> The previous commit message, along with comments elsewhere, assert\n>> that the WIN32 macro is not defined on Cygwin. Currently, this is\n>> true for the cygwin build. However, the cygwin platform can be\n>> used to develop WIN32 GUI, WIN32 console, and POSIX applications.\n>> Indeed it is possible to create applications which use a mix of\n>> the WIN32 API and POSIX code (eg git!).\n> \n> In this paragraph, you are only saying that cygwin comes with headers and\n> libraries that can be used to write code using the Windows API in addition\n> to the POSIX headers and libraries. (I'm just asking, not complaining;\n> perhaps this could be stated differently.)\n> \n\n;-) The above paragraph, along with the one below, was once part of an even\nlarger paragraph which I had edited multiple times and, eventually, split\nup. I had intended to delete the last two sentences from the above paragraph.\nWhilst the content of those sentences is true, it is not necessary for them\nto be there and, having been orphaned at the end of the paragraph, now sound\na bit odd.\n\nIf I were to create a version 4 of the patch, I would delete that text.\n\n>> Unlike native WIN32 compilers, gcc on cygwin does not automatically\n>> define the _WIN32 macro. However, as soon as you include the\n>> header file, the _WIN32 and WIN32 macros are defined.\n>>\n>> In order to reduce the risk of problems in the future, we protect\n>> the inclusion of the windows header with an explicit check for\n>> __CYGWIN__. Also, we move the other use of the header\n>> from compat/win32.h to compat/cygwin.c\n> \n> But I sense a contradiction here. Above you are arguing that much more\n> WIN32 code is included, but here you are saying that the explicit check\n---------------^\npotentially\n\n> for __CYGWIN__ is just a safety measure to protect us from failures in\n> future changes. Indeed, looking at the code it seems that this extra check\n> is *currently* not necessary:\n\n*correct*! I have not claimed otherwise.\n\n[snipped other text I also agree with!]\n\n> IOW, I disagree with your analysis that a lot of code suffers from\n> windows.h pollution. What am I missing?\n\nHmm, I don't know; but the analysis that you claim for me, I don't\nthink is mine! :-D\n\nHmm, I don't have a great investment in this patch (it doesn't fix a bug\nafter all), so given that you don't see any merit in it, I'm more than\nhappy just to drop it! :-P\n\nSo, Junio, could you please drop this patch from the series.\n\nThanks!\n\nATB,\nRamsay Jones\n"}]}