[libc-commits] [PATCH] D116777: [libc] Add rounding mode support for MPFR testing macros.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.h:291
 
-#define EXPECT_MPFR_MATCH(op, input, match_value, tolerance)                   \
+#define MISSING_ARGS(...) static_assert(true, "Missing arguments");
+
----------------
sivachandra wrote:
> lntue wrote:
> > sivachandra wrote:
> > > lntue wrote:
> > > > sivachandra wrote:
> > > > > I am not sure if this `MISSING_ARGS` macro is any useful. For, if any error occurs, it will print an error message but tell us nothing about what is actually wrong. I would think a better approach would be to let the `RoundingMode` arg to `get_mpfr_matcher` take a default value of `RoundingMode::Nearest`. That way, you can also simplify the macros below to some thing like this:
> > > > > 
> > > > > ```
> > > > > #define EXPECT_MPFR_MATCH(op, input, match_value, tolerance, ...) \
> > > > >   EXPECT_THAT(match_value,                                        \
> > > > >               __llvm_libc::testing::mpfr::get_mpfr_matcher<op>(   \
> > > > >                   input, match_value, tolerance                   \
> > > > >                   __VA_OPT__(,) __VA_ARGS__))
> > > > > ```
> > > > > 
> > > > > This way, if there is any error in the args, we will actually get a proper compiler error message. If using `__VA_OPT__` is a concern, we can use `-std=c++20` for compiling MPFRWrapper.
> > > > I use MISSING_ARGS as a simple way to avoid gnu-zero-variadic-macro-arguments warnings.  It is simpler and does not require extra compiler flags as __VA_OPT__ option.  I changed it to GET_MPFR_DUMMY_ARG and added comments to make the intention clearer.
> > > Ah! So, why should `GET_MPFR_MACRO` be a variadic macro at all?
> > To support and not litter all current usages of EXPECT_MPFR_MATCH with default rounding mode as the 5th argument, it is expanded to either EXPECT_MPFR_MATCH_ROUNDING with 5 arguments, or EXPECT_MPFR_MATCH_DEFAULT with 4 arguments by GET_MPFR_MACRO.
> > 
> > If 5 arguments are provided to EXPECT_MPFR_MATCH, GET_MPFR_MACRO will get 6 arguments (not including GET_MPFR_DUMMY_ARG), and if 4 arguments are provided to EXPECT_MPFR_MATCH, GET_MPFR_MACRO will only get 5 arguments (not including GET_MPFR_DUMMY_ARG), and hence it needs to be variadic.  
> > 
> > It is the 4 argument case that variadic GET_MPFR_MACRO got complained by -Wgnu-zero-variadic-macro-arguments without GET_MPFR_DUMMY_ARG.
> So, the way you have set it up, it can already support 4 or 5 args. But, it need not be a variadic macro. FWIW, I patched your change and removed the `...` and also the last arg to the macro on line 309 and line 325. It goes through fine.
Because there were no tests using the updated macros with rounding parameters.  I've added simple tests for sqrt with different rounding modes.  Now the build would fail if you remove ... from GET_MPFR_MACRO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116777/new/

https://reviews.llvm.org/D116777