[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 {};
 
----------------
lenary wrote:
> fpetrogalli wrote:
> > Is this needed? 
> My original intention was to do the minimal changes required to just move the code.
> 
> It's not technically needed, so I will remove it.
Yeah, I totally get the idea of doing minimal changes. Juts mention in the commit message that you have some extra cleanup that is not strictly needed for the major changes of the patch, so that people blaming the change can understand why that was done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137836