Re: [PATCH v2 4/4] Add explicit Cygwin check to guard WIN32 header inclusion

[Ramsay Jones <ramsay@ramsay1.demon.co.uk>]

Johannes Sixt wrote:
> Ramsay Jones schrieb:
>> Since commit 435bdf8c ("Make usage of windows.h lean and mean",
>> 16-9-2009), the amount of code potentially including the WIN32
>> API header files has greatly increased. In particular, the Cygwin
>> build is at greater risk of inadvertently including WIN32 code
>> within preprocessor sections protected by the WIN32 or _WIN32
>> macros.
> 
> Thanks, this makes the problem pretty clear that you want to solve.
> 

Hmm, OK. Note the "... potentially including ..." above.

>> The previous commit message, along with comments elsewhere, assert
>> that the WIN32 macro is not defined on Cygwin. Currently, this is
>> true for the cygwin build. However, the cygwin platform can be
>> used to develop WIN32 GUI, WIN32 console, and POSIX applications.
>> Indeed it is possible to create applications which use a mix of
>> the WIN32 API and POSIX code (eg git!).
> 
> In this paragraph, you are only saying that cygwin comes with headers and
> libraries that can be used to write code using the Windows API in addition
> to the POSIX headers and libraries. (I'm just asking, not complaining;
> perhaps this could be stated differently.)
> 

;-) The above paragraph, along with the one below, was once part of an even
larger paragraph which I had edited multiple times and, eventually, split
up. I had intended to delete the last two sentences from the above paragraph.
Whilst the content of those sentences is true, it is not necessary for them
to be there and, having been orphaned at the end of the paragraph, now sound
a bit odd.

If I were to create a version 4 of the patch, I would delete that text.

>> Unlike native WIN32 compilers, gcc on cygwin does not automatically
>> define the _WIN32 macro. However, as soon as you include the
>> <windows.h> header file, the _WIN32 and WIN32 macros are defined.
>>
>> In order to reduce the risk of problems in the future, we protect
>> the inclusion of the windows header with an explicit check for
>> __CYGWIN__. Also, we move the other use of the <windows.h> header
>> from compat/win32.h to compat/cygwin.c
> 
> But I sense a contradiction here. Above you are arguing that much more
> WIN32 code is included, but here you are saying that the explicit check
---------------^
potentially

> for __CYGWIN__ is just a safety measure to protect us from failures in
> future changes. Indeed, looking at the code it seems that this extra check
> is *currently* not necessary:

*correct*! I have not claimed otherwise.

[snipped other text I also agree with!]

> IOW, I disagree with your analysis that a lot of code suffers from
> windows.h pollution. What am I missing?

Hmm, I don't know; but the analysis that you claim for me, I don't
think is mine! :-D

Hmm, I don't have a great investment in this patch (it doesn't fix a bug
after all), so given that you don't see any merit in it, I'm more than
happy just to drop it! :-P

So, Junio, could you please drop this patch from the series.

Thanks!

ATB,
Ramsay Jones