[PATCH] D137836: [Support] Move getHostNumPhysicalCores to Threading.h

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
Comment at: llvm/unittests/Support/Host.cpp:33
 
-class HostTest : public testing::Test {
-  Triple Host;
-
-protected:
-  bool isSupportedArchAndOS() {
-    // Initially this is only testing detection of the number of
-    // physical cores, which is currently only supported/tested on
-    // some systems.
-    return (Host.isOSWindows() && llvm_is_multithreaded()) ||
-           Host.isOSDarwin() || (Host.isX86() && Host.isOSLinux()) ||
-           (Host.isOSLinux() && !Host.isAndroid()) ||
-           (Host.isSystemZ() && Host.isOSzOS());
-  }
-
-  HostTest() : Host(Triple::normalize(sys::getProcessTriple())) {}
-};
-
-TEST_F(HostTest, NumPhysicalCoresSupported) {
-  if (!isSupportedArchAndOS())
-    GTEST_SKIP();
-  int Num = sys::getHostNumPhysicalCores();
-  ASSERT_GT(Num, 0);
-}
-
-TEST_F(HostTest, NumPhysicalCoresUnsupported) {
-  if (isSupportedArchAndOS())
-    GTEST_SKIP();
-  int Num = sys::getHostNumPhysicalCores();
-  ASSERT_EQ(Num, -1);
-}
+class HostTest : public testing::Test {};
 
----------------
Is this needed? 


================
Comment at: llvm/unittests/Support/Host.cpp:401
 
 TEST_F(HostTest, DummyRunAndGetCommandOutputUse) {
   // Suppress defined-but-not-used warnings when the tests using the helper are
----------------
I think you can remove the class `HostTest` and convert the test fixture into just `TEST(` instead of `TEST_F(`. It is kind of awkward to have this: `class HostTest : public testing::Test {};`


================
Comment at: llvm/unittests/Support/Threading.cpp:30
 
+class ThreadingTest : public testing::Test {
+  Triple Host;
----------------
I don't think you need to derive from `testing:Test`? The functionality of `isSupportedArchAndOS could be achieved just with a function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137836