Re: [Qemu-devel] [PATCH v2 1/5] w32: Add and use intermediate include file for windows.h

[Stefan Weil]

Am 10.03.2014 09:56, schrieb Markus Armbruster:
> Stefan Weil <address@hidden> writes:
> 
>> Including windows.h from the new file include/qemu/winapi.h allows
>> better tracking of the files which depend on the Windows API.
>>
>> 1864 *.o files depend on windows.h in a typical build, only 88 *.o files
>> don't.
>>
>> The windows.h specific macro WIN32_LEAN_AND_MEAN is now defined in the new
>> file and no longer part of the QEMU_CFLAGS. A hack in ui/sdl.c can be
>> removed after this change.
>>
>> WINVER is still needed for all compilations with MinGW, so it cannot be
>> defined in the new file. Replace its numeric value by a symbolic value to
>> show which API is requested.
> [...]
>> diff --git a/block/raw-win32.c b/block/raw-win32.c
>> index ae1c8e6..95b27a5 100644
>> --- a/block/raw-win32.c
>> +++ b/block/raw-win32.c
>> @@ -23,13 +23,13 @@
>>   */
>>  #include "qemu-common.h"
>>  #include "qemu/timer.h"
>> +#include "qemu/winapi.h"        /* HANDLE (in raw-aio.h) */
> 
> Such comments get out of date real fast.  I treat them as noise.

I agree that dependencies change over time. That's the main reason for
this kind of comments: it is easy to check whether the comment still
applies. If not, chances are good that the include statement is no
longer needed and can be removed together with the comment. How can
human reviewers see quickly why an include statement is needed when
there is no comment? Usually they can see that only for the most common
cases, and for all others they need help from the compiler.

> 
> If raw-aio.h needs stuff from winapi.h, why doesn't raw-aio.h include
> it?

It could, of course, but I assume that there is no special reason why it
does not. A lot of QEMU sources don't include everything which they need
explicitly. I thought about moving the include statement from
block/raw-win32.c and block/win32-aio.c to raw-aio.h, but decided to
minimize my changes and keep the current structure.

Stefan