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

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added inline comments.
This revision is now accepted and ready to land.


================
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();
----------------
lntue wrote:
> 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.
Ah - this sounds like, if you really need more parameterization in future, then we should parameterize the test class and not the test method? As in,

```
template <mpfr::RoundingMode Rounding>
class LlvmLibcLogfExhaustiveTest : public LlvmLibcExhaustiveTest<uint32_t> {
  ...
};
```

Virtual methods are there essentially there to avoid such "poor man's C++" one can setup using member function pointers. So, I think this virtual function based solution is more idiomatic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117028