[PATCH v2 6/9] eal: expand most macros to empty when using msvc

[Tyler Retzlaff]

On Wed, Apr 05, 2023 at 11:44:43AM +0100, Bruce Richardson wrote:
> On Tue, Apr 04, 2023 at 01:07:24PM -0700, Tyler Retzlaff wrote:
> > For now expand a lot of common rte macros empty. The catch here is we
> > need to test that most of the macros do what they should but at the same
> > time they are blocking work needed to bootstrap of the unit tests.
> > 
> > Later we will return and provide (where possible) expansions that work
> > correctly for msvc and where not possible provide some alternate macros
> > to achieve the same outcome.
> > 
> > Signed-off-by: Tyler Retzlaff <roretzla at linux.microsoft.com>
> > ---
> >  lib/eal/include/rte_branch_prediction.h |  8 ++++++++
> >  lib/eal/include/rte_common.h            | 33 +++++++++++++++++++++++++++++++++
> >  lib/eal/include/rte_compat.h            | 20 ++++++++++++++++++++
> >  3 files changed, 61 insertions(+)
> > 
> > diff --git a/lib/eal/include/rte_branch_prediction.h b/lib/eal/include/rte_branch_prediction.h
> > index 0256a9d..3589c97 100644
> > --- a/lib/eal/include/rte_branch_prediction.h
> > +++ b/lib/eal/include/rte_branch_prediction.h
> > @@ -25,7 +25,11 @@
> >   *
> >   */
> >  #ifndef likely
> > +#ifndef RTE_TOOLCHAIN_MSVC
> >  #define likely(x)	__builtin_expect(!!(x), 1)
> > +#else
> > +#define likely(x)	(!!(x) == 1)
> > +#endif
> 
> I think this should just be "#define likely(x) (x)", since the likely is
> just a hint as to which way we expect the branch to go. It does not change
> the logic in the expression.
> 
> >  #endif /* likely */
> >  
> >  /**
> > @@ -39,7 +43,11 @@
> >   *
> >   */
> >  #ifndef unlikely
> > +#ifndef RTE_TOOLCHAIN_MSVC
> >  #define unlikely(x)	__builtin_expect(!!(x), 0)
> > +#else
> > +#define unlikely(x)	(!!(x) == 0)
> > +#endif
> 
> This expansion is wrong, because it changes the logic of the expression,
> rather than being a hint. As above with likely, I think this should just
> expand as "(x)", making the unlikely ignored.

ooh, obviously wrong will fix.

> 
> >  #endif /* unlikely */
> >  
> >  #ifdef __cplusplus
> > diff --git a/lib/eal/include/rte_common.h b/lib/eal/include/rte_common.h
> > index 2f464e3..a724e22 100644
> > --- a/lib/eal/include/rte_common.h
> > +++ b/lib/eal/include/rte_common.h
> 
> This file has a lot of ifdefs in it now for msvc. Couple of suggestions:
> 
> 1. can we group these defines together so we can hit multiple entries with a
>   single msvc block?
> 
> 2. alternatively, for those we want to just permanently null, we could
>    split the responsibility for them between the currnet headers and possibly
>    the build system. Specifically, rather than doing macros based on MSVC,
>    change each macro to the simpler:
> 
>    #ifndef MACRO
>    #define MACRO  macro_definition
>    #endif


> 
>     Then in the meson.build processing, we can have some separte MSVC
>     processing to put null defines for these into rte_build_config.h, or put
>     in null defines for them in some MSVC-specific header.

yes, i'll do this for now. i won't go the route of rte_build_config.h
for now since some of the macros i will eventually try to find a way to
provide equivalent expansion.

> 
> Just some thoughts.
> /Bruce