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 <roret...@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
