Add an extra argument for rounding mode to EXPECT_MPFR_MATCH and ASSERT_MPFR_MATCH macros.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/utils/MPFRWrapper/MPFRUtils.h | ||
---|---|---|
306–319 | 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. |
libc/utils/MPFRWrapper/MPFRUtils.h | ||
---|---|---|
306–319 | 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. |
libc/utils/MPFRWrapper/MPFRUtils.h | ||
---|---|---|
306–319 | 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. |
libc/utils/MPFRWrapper/MPFRUtils.h | ||
---|---|---|
306–319 | 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. |
Add simple tests for sqrt with different rounding modes.
Make all the MPFR util functions accept both precision and rounding mode.
libc/utils/MPFRWrapper/MPFRUtils.h | ||
---|---|---|
306–319 | 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. |
libc/utils/MPFRWrapper/MPFRUtils.cpp | ||
---|---|---|
149–152 | Nit: you should probably use XType here and below instead of float etc. | |
libc/utils/MPFRWrapper/MPFRUtils.h | ||
306–319 | 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. | |
310 | This is still not addressed. |
libc/test/src/math/SqrtTest.h | ||
---|---|---|
69 | I am assuming this is just a demo and that you will remove it before submitting. |
I am assuming this is just a demo and that you will remove it before submitting.