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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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);
+};
----------------
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.


================
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:
> > 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.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:558
   MPFRNumber mpfrMatchValue(matchValue);
-  OS << "Match value not within tolerance value of MPFR result:\n"
+  std::stringstream ss;
+  ss << "Match value not within tolerance value of MPFR result:\n"
----------------
lntue wrote:
> sivachandra wrote:
> > It's not clear to me as to why you need a `stringstream`. Can you explain why?
> stringstream is needed so that the error messages are printed without interleaving each other when running concurrently.
Ah, thanks. Can you add a note at a suitable place in this file?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117028