[PATCH v3 06/11] eal: typedef cpu flag enum as int for msvc

[Konstantin Ananyev]

17/04/2023 16:46, Tyler Retzlaff пишет:
> On Sun, Apr 16, 2023 at 10:29:38PM +0100, Konstantin Ananyev wrote:
>> 10/04/2023 21:53, Tyler Retzlaff пишет:
>>> On Mon, Apr 10, 2023 at 08:59:33PM +0100, Konstantin Ananyev wrote:
>>>> 06/04/2023 01:45, Tyler Retzlaff пишет:
>>>>> Forward declaration of a typedef is a non-standard extension and is not
>>>>> supported by msvc. Use an int instead.
>>>>>
>>>>> Abstract the use of the int/enum rte_cpu_flag_t in function parameter
>>>>> lists by re-typdefing the enum rte_cpu_flag_t to the rte_cpu_flag_t
>>>>> identifier.
>>>>>
>>>>> Remove the use of __extension__ on function prototypes where
>>>>> rte_cpu_flag_t appeared in parameter lists, it is sufficient to have the
>>>>> conditionally compiled __extension__ at the non-standard forward
>>>>> declaration site.
>>>>>
>>>>> Signed-off-by: Tyler Retzlaff <roretzla at linux.microsoft.com>
>>>>> ---
>>>>>   lib/eal/include/generic/rte_cpuflags.h | 12 +++++++-----
>>>>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/lib/eal/include/generic/rte_cpuflags.h b/lib/eal/include/generic/rte_cpuflags.h
>>>>> index d35551e..87ab03c 100644
>>>>> --- a/lib/eal/include/generic/rte_cpuflags.h
>>>>> +++ b/lib/eal/include/generic/rte_cpuflags.h
>>>>> @@ -44,8 +44,12 @@ struct rte_cpu_intrinsics {
>>>>>   /**
>>>>>    * Enumeration of all CPU features supported
>>>>>    */
>>>>> +#ifndef RTE_TOOLCHAIN_MSVC
>>>>>   __extension__
>>>>> -enum rte_cpu_flag_t;
>>>>> +typedef enum rte_cpu_flag_t rte_cpu_flag_t;
>>>>> +#else
>>>>> +typedef int rte_cpu_flag_t;
>>>>> +#endif
>>>>
>>>>
>>>> Just curious what exactly MSVC doesn't support here?
>>>> Is that construction like:
>>>> enum rte_cpu_flag_t {....};
>>>> enum rte_cpu_flag_t;
>>>> ...
>>>> Or something else?
>>>
>>> Forward declaration of an enum is non standard. It's probably only
>>> allowed by gcc as an extension because gcc will make some kind of
>>> implementation specific promise for it always to be `int` size by
>>> default (assuming no other -foptions).
>>
>> I understood that part, what is not clear to me from where we are
>> getting forward declarations?
> 
> i investigated and expirmented with this further. i can see now there
> were two things that were tripping things up portability wise.
> 
> * it's still a forward declaration even if the full enum definition is
>    visible and thus generates a warning (but not an error).
> 
> * the use of __extension__ which is valid in this context because it is
>    after all an extension (causes compilation error for msvc).
> 
> my testing shows i'm still getting correct sizes (because the definition
> is visible) so what i'll do is update this patch to remove the use of
> __extension__ and allow the warning to be emitted for now. i think this
> is probably what you're preference would be.
> 
> in a future series i will either suppress the warning for msvc or remove
> the declaration entirely which as you point out should not be needed due
> to the include via arch/rte_cpuflags.h -> include generic/rte_cpuflags.h.

Agree, let's try to remove this declaration.
I don't see any point to have declaration straight after defintion.
Thanks
Konstantin

> 
> thanks!
> 
>> As I remember the usual organization of arch specific rte_cpuflags.h:
>>
>> /* type definition */
>> enum rte_cpu_flag_t {...};
>>
>> /some other stuff */
>>
>> #include "generic/rte_cpuflags.h"
>>
>> Which contains 'enum rte_cpu_flag_t' type declaration.
>> But it doesn't look like a forward declaration to me.
>> Is there a place where we do include "generic/rte_cpuflags.h" directly
>> (not from arch specific header)?
>> If so. might be we should change it to include arch specific header instead?
>>
>>>
>>> If the enum was defined before reference it would probably be accepted
>>> by msvc since it could 'see' the definition and know the integer width
>>> in use.
>>>
>>>>
>>>>
>>>>>   /**
>>>>>    * Get name of CPU flag
>>>>> @@ -56,9 +60,8 @@ struct rte_cpu_intrinsics {
>>>>>    *     flag name
>>>>>    *     NULL if flag ID is invalid
>>>>>    */
>>>>> -__extension__
>>>>>   const char *
>>>>> -rte_cpu_get_flag_name(enum rte_cpu_flag_t feature);
>>>>> +rte_cpu_get_flag_name(rte_cpu_flag_t feature);
>>>>>   /**
>>>>>    * Function for checking a CPU flag availability
>>>>> @@ -70,9 +73,8 @@ struct rte_cpu_intrinsics {
>>>>>    *     0 if flag is not available
>>>>>    *     -ENOENT if flag is invalid
>>>>>    */
>>>>> -__extension__
>>>>>   int
>>>>> -rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature);
>>>>> +rte_cpu_get_flag_enabled(rte_cpu_flag_t feature);
>>>>>   /**
>>>>>    * This function checks that the currently used CPU supports the CPU features