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

[Sam Elliott via Phabricator via cfe-commits]

lenary 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 {};
 
----------------
fpetrogalli wrote:
> 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.
So, I forgot to submit this comment *before* I submitted the changes. If you're happy with how the patch looks now, +2 and I'll land it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137836