[libc-commits] [PATCH] D117028: [libc] Add multithreading support for exhaustive testing and MPFRUtils.

[Tue Ly via Phabricator via libc-commits]

lntue marked an inline comment as done.
lntue added inline comments.


================
Comment at: libc/test/src/math/exhaustive/exhaustive_test.h:26
+  // Pointer to the testing method to be used by `test_full_range`.
+  void (LlvmLibcExhaustiveTest::*check)(T, T);
+};
----------------
sivachandra wrote:
> Why is using a method pointer over an overridable virtual method with an additional rounding mode arg better? FWIW, method pointers are like virtual functions.
See the comment below in logf_test.cpp.


================
Comment at: libc/test/src/math/exhaustive/logf_test.cpp:26
+  template <mpfr::RoundingMode Rounding>
+  void check(uint32_t start, uint32_t stop) {
+    int old_rounding = fegetround();
----------------
sivachandra wrote:
> lntue wrote:
> > sivachandra wrote:
> > > It seems to me that this function need not be a template? As in, the rounding mode can be an arg? You will need to add a rounding mode arg to `testFullRange` also I think.
> > I don't think rounding mode information should be included in test_full_range, as their sole purpose is to break down the input range and run it in parallel.  So in order to move test_full_range implementation to .cpp file, the testing function has to have a fixed signature.  That leave template parameter as the cleanest way to pass rounding mode to each testing function.
> I am not sure what "including" rounding mode information in `test_full_range` would me. But, with the virtual function setup, it will be like this:
> 
> ```
> test_full_range(..., RoundingMode rounding_mode) {
>   check(..., rounding_mode);
> }
> ```
> 
> And it will avoid all the function pointers in the tests below. Also, the template arg is not deducible so there is not benefit to making it a template.
> 
> An additional note:  Tests which don't need rounding mode will just ignore that arg - so there has to be a default.
It's exactly passing rounding_mode directly to test_full_range that I didn't like and preferred method pointer over virtual function.  Since if other tests need different parameters to execute, similar to rounding_mode, then they will have to pass to test_full_range, and all other overridden functions would have to be updated to carry more unused parameters. 

Without template parameters (since its implementations is not in the header) and std::function, I found method pointers is the next easiest way to allow each test to add their own required parameters individually without affecting other tests.

Nonetheless, I updated to use virtual function in here, and we can change to method pointers later if more tests need different parameters.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117028