[Mesa-dev] [PATCH] st/nine: correctly fold guards around define WINAPI

[Emil Velikov]

On 15 April 2016 at 22:11, Grazvydas Ignotas <notasas at gmail.com> wrote:
> On Fri, Apr 15, 2016 at 9:46 PM, Christian Schmidbauer
> <ch.schmidbauer at gmail.com> wrote:
>> On 15/04/16 17:06, Emil Velikov wrote:
>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>
>>> The __i386__ and __x86-64__ macros are gcc/clang specific, thus one does
>>> not need the __GNUC__ at the top.
>>>
>>> Additionally, having _M_IX86 and _M_X64 in the same block (and even use
>>> __attribute__(foo)) is wrong as those are set by MSVC.
>>>
>>> If at some point we do start building with the Sun/Oracle compiler we
>>> might need to add the __i386 and __x86-64 and explicit checks for
>>> __attribute__(foo) as the latter is not something that exists there.
>>>
>>> Cc: Christian Schmidbauer <ch.schmidbauer at gmail.com>
>>> Cc: Axel Davy <axel.davy at ens.fr>
>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>> ---
>>>  include/D3D9/d3d9types.h | 16 +++++++---------
>>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/D3D9/d3d9types.h b/include/D3D9/d3d9types.h
>>> index dce5533..bcce061 100644
>>> --- a/include/D3D9/d3d9types.h
>>> +++ b/include/D3D9/d3d9types.h
>>> @@ -178,15 +178,13 @@ typedef struct _RGNDATA {
>>>  #undef WINAPI
>>>  #endif /* WINAPI*/
>>>
>>> -#ifdef __GNUC__
>>> -  #if (defined(__x86_64__) && !defined(__ILP32__)) || defined(_M_X64)
>>> -    #define WINAPI __attribute__((ms_abi))
>>> -  #elif defined(__i386) || defined(_M_IX86)
>>> -    #define WINAPI __attribute__((__stdcall__))
>>> -  #else /* neither amd64 nor i386 */
>>> -    #define WINAPI
>>> -  #endif
>>> -#else /* __GNUC__ */
>>> +#if defined(__x86_64__) && !defined(__ILP32__)
>>> +  #define WINAPI __attribute__((ms_abi))
>>> +#elif defined(__i386__)
>>> +  #define WINAPI __attribute__((__stdcall__))
>>> +#elif defined(_M_IX86) /* MSVC specific, do we even use it ? */
>>> +  #define WINAPI
>>> +#elif defined(_M_X64) /* MSVC specific, do we even use it ? */
>>>    #define WINAPI
>>>  #endif
>>>
>>
>> You forgot the last "#else ... #define WINAPI", which is the important
>> part/build fix on other abis than i386 and amd64.
>> Apart from that, removing GNUC (as only gcc, clang and MSVC are
>> supported by Mesa) looks fine to me. The original intention was to
>> keep the patch as compatible with as many compilers as I could think
>> of, hence the second guard.
>>
>> About the MSVC bits:
>> I can think only of two scenarios for having a WINAPI define beforehand:
>>
>> 1) Some other code needs to be compatible with Windows. Silently
>> undefining and re-defining WINAPI in such a case might break their
>> code, but Nine (and hence Wine) will run properly. I guess this was
>> the intention of the original author.
>>
>> 2) You are compiling Nine with MSVC. MSVC already defines the proper
>> WINAPI, so I assume you don't want to remove WINAPI in such a case.
>
> That's not entirely accurate, WINAPI is defined by platform headers,
> not the compiler itself, so _MSC_VER checking would not be correct.
> But yeah if something defined WINAPI already, it might be reasonable
> to assume they know what they're doing and leave it alone.
>
I'd rather leave decision like "should we allow building nine with
MSVC" and "should we keep/correctly set the MSVC macros" to the people
working/maintaining the code.

Gents, feel free to pick up the patch and tweak to your liking or
throw it out the window and come with a better solution.

-Emil