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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:149
             cpp::EnableIfType<cpp::IsSame<float, XType>::Value, int> = 0>
-  explicit MPFRNumber(XType x, int precision = 128)
-      : mpfr_precision(precision) {
+  explicit MPFRNumber(XType x, int precision = ExtraPrecision<float>::VALUE,
+                      RoundingMode rounding = RoundingMode::Nearest)
----------------
Nit: you should probably use `XType` here and below instead of `float` etc.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.h:295
+
+#define GET_MPFR_MACRO(_1, _2, _3, _4, _5, NAME, ...) NAME
+
----------------
sivachandra wrote:
> Add double-underscore prefix to internal macro names.
This is still not addressed.


================
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");
+
----------------
lntue wrote:
> 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.
Thanks! That helps me see it better. I still prefer to keep it simple really. The new macro can be called `ASSERT_MPFR_MATCH_WITH_ROUNDING` and make it explicit. I agree it is verbose, but keeps everything really simple. For, the error message I see now says that there are incorrect number of args somewhere, but does not tell me where exactly. I will leave it up to you do decide.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116777