Fix header-filter for clang-tidy c10 and apply some fixes to c10 and … (

pytorch#91178) …c10d Fixes a broken header filters from pytorch#90699 and applies a few more clang-tidy fixes that are relevant from c10 and c10d. The header filter pattern was actually broken and the clang-tidy include pattern was redundant. Also fixed a few bugs in torch/distributed/c10d Pull Request resolved: pytorch#91178 Approved by: https://github.com/ezyang
Chao1Han · Dec 27, 2022 · 97db9fd · 97db9fd
1 parent bb24185
commit 97db9fd
Show file tree

Hide file tree

Showing 28 changed files with 67 additions and 60 deletions.
diff --git a/.clang-tidy b/.clang-tidy
@@ -38,7 +38,7 @@ performance-*,
 -performance-noexcept-move-constructor,
 -performance-unnecessary-value-param,
 '
-HeaderFilterRegex: '(c10/(?!test)/|torch/csrc/(?!deploy/interpreter/cpython)).*'
+HeaderFilterRegex: '^(c10/(?!test)|torch/csrc/(?!deploy/interpreter/cpython)).*$'
 AnalyzeTemporaryDtors: false
 WarningsAsErrors: '*'
 CheckOptions:

diff --git a/.lintrunner.toml b/.lintrunner.toml
@@ -211,7 +211,6 @@ command = [
 [[linter]]
 code = 'CLANGTIDY'
 include_patterns = [
-    'c10/core/*.cpp',
     'c10/core/**/*.cpp',
     'torch/csrc/fx/**/*.cpp',
     'torch/csrc/generic/**/*.cpp',

diff --git a/c10/mobile/CPUCachingAllocator.cpp b/c10/mobile/CPUCachingAllocator.cpp
@@ -97,8 +97,8 @@ CPUCachingAllocator* GetThreadLocalCachingAllocator() {
 }
 
 WithCPUCachingAllocatorGuard::WithCPUCachingAllocatorGuard(
-    CPUCachingAllocator* allocator) {
-  prev_caching_allocator_ptr_ = GetThreadLocalCachingAllocator();
+    CPUCachingAllocator* allocator)
+    : prev_caching_allocator_ptr_(GetThreadLocalCachingAllocator()) {
   caching_allocator_ptr = allocator;
 }
 

diff --git a/c10/util/Exception.cpp b/c10/util/Exception.cpp
@@ -7,6 +7,7 @@
 #include <numeric>
 #include <sstream>
 #include <string>
+#include <utility>
 
 namespace c10 {
 
@@ -183,27 +184,27 @@ void warn(const Warning& warning) {
 Warning::Warning(
     warning_variant_t type,
     const SourceLocation& source_location,
-    const std::string& msg,
+    std::string msg,
     const bool verbatim)
     : type_(type),
       source_location_(source_location),
-      msg_(msg),
+      msg_(std::move(msg)),
       verbatim_(verbatim) {}
 
 Warning::Warning(
     warning_variant_t type,
     SourceLocation source_location,
     detail::CompileTimeEmptyString msg,
     const bool verbatim)
-    : Warning(type, std::move(source_location), "", verbatim) {}
+    : Warning(type, source_location, "", verbatim) {}
 
 Warning::Warning(
     warning_variant_t type,
     SourceLocation source_location,
     const char* msg,
     const bool verbatim)
     : type_(type),
-      source_location_(std::move(source_location)),
+      source_location_(source_location),
       msg_(std::string(msg)),
       verbatim_(verbatim) {}
 

diff --git a/c10/util/Exception.h b/c10/util/Exception.h
@@ -123,7 +123,7 @@ class C10_API Warning {
   Warning(
       warning_variant_t type,
       const SourceLocation& source_location,
-      const std::string& msg,
+      std::string msg,
       bool verbatim);
 
   Warning(

diff --git a/c10/util/SmallVector.cpp b/c10/util/SmallVector.cpp
@@ -126,7 +126,7 @@ void SmallVectorBase<Size_T>::grow_pod(
     size_t MinSize,
     size_t TSize) {
   size_t NewCapacity = getNewCapacity<Size_T>(MinSize, TSize, this->capacity());
-  void* NewElts;
+  void* NewElts = nullptr;
   if (BeginX == FirstEl) {
     NewElts = std::malloc(NewCapacity * TSize);
     if (NewElts == nullptr) {

diff --git a/c10/util/int128.cpp b/c10/util/int128.cpp
@@ -129,7 +129,7 @@ std::ostream& operator<<(std::ostream& o, const uint128& b) {
 
   // Select a divisor which is the largest power of the base < 2^64.
   uint128 div;
-  std::streamsize div_base_log;
+  std::streamsize div_base_log = 0;
   switch (flags & std::ios::basefield) {
     case std::ios::hex:
       div = (uint64_t)0x1000000000000000u; // 16^15

diff --git a/c10/util/numa.cpp b/c10/util/numa.cpp
@@ -49,7 +49,7 @@ int GetNUMANode(const void* ptr) {
   TORCH_CHECK(
       get_mempolicy(
           &numa_node,
-          NULL,
+          nullptr,
           0,
           const_cast<void*>(ptr),
           MPOL_F_NODE | MPOL_F_ADDR) == 0,

diff --git a/c10/util/signal_handler.cpp b/c10/util/signal_handler.cpp
@@ -57,7 +57,7 @@ void hookupHandler() {
   if (hookedUpCount++) {
     return;
   }
-  struct sigaction sa;
+  struct sigaction sa {};
   // Setup the handler
   sa.sa_handler = &handleSignal;
   // Restart the system call, if at all possible
@@ -78,7 +78,7 @@ void unhookHandler() {
   if (--hookedUpCount > 0) {
     return;
   }
-  struct sigaction sa;
+  struct sigaction sa {};
   // Setup the sighub handler
   sa.sa_handler = SIG_DFL;
   // Restart the system call, if at all possible
@@ -106,7 +106,7 @@ FatalSignalHandler& FatalSignalHandler::getInstance() {
   return *handler;
 }
 
-FatalSignalHandler::~FatalSignalHandler() {}
+FatalSignalHandler::~FatalSignalHandler() = default;
 
 FatalSignalHandler::FatalSignalHandler()
     : fatalSignalHandlersInstalled(false),
@@ -205,7 +205,7 @@ void FatalSignalHandler::fatalSignalHandler(int signum) {
   if (procDir) {
     pid_t pid = getpid();
     pid_t currentTid = syscall(SYS_gettid);
-    struct dirent* entry;
+    struct dirent* entry = nullptr;
     pthread_mutex_lock(&writingMutex);
     while ((entry = readdir(procDir)) != nullptr) {
       if (entry->d_name[0] == '.') {
@@ -263,7 +263,7 @@ void FatalSignalHandler::installFatalSignalHandlers() {
     return;
   }
   fatalSignalHandlersInstalled = true;
-  struct sigaction sa;
+  struct sigaction sa {};
   sigemptyset(&sa.sa_mask);
   // Since we'll be in an exiting situation it's possible there's memory
   // corruption, so make our own stack just in case.

diff --git a/c10/util/signal_handler.h b/c10/util/signal_handler.h
@@ -78,7 +78,7 @@ class TORCH_API FatalSignalHandler {
   bool fatalSignalHandlersInstalled;
   // We need to hold a reference to call the previous SIGUSR2 handler in case
   // we didn't signal it
-  struct sigaction previousSigusr2;
+  struct sigaction previousSigusr2 {};
   // Flag dictating whether the SIGUSR2 handler falls back to previous handlers
   // or is intercepted in order to print a stack trace.
   std::atomic<bool> fatalSignalReceived;

diff --git a/torch/csrc/distributed/c10d/Backend.cpp b/torch/csrc/distributed/c10d/Backend.cpp
@@ -9,7 +9,7 @@ Backend::Backend(int rank, int size)
   C10_LOG_API_USAGE_ONCE("c10d.backend");
 }
 
-Backend::~Backend() {}
+Backend::~Backend() = default;
 
 void Backend::init() {
   C10_LOG_API_USAGE_ONCE(fmt::format("c10d.backend_{}", getBackendName()));

diff --git a/torch/csrc/distributed/c10d/FileStore.cpp b/torch/csrc/distributed/c10d/FileStore.cpp
@@ -1,9 +1,9 @@
 #include <torch/csrc/distributed/c10d/FileStore.hpp>
 
-#include <assert.h>
 #include <fcntl.h>
-#include <stdint.h>
 #include <sys/stat.h>
+#include <cassert>
+#include <cstdint>
 
 #ifdef _WIN32
 #include <c10/util/win32-headers.h>
@@ -22,6 +22,7 @@
 #include <sstream>
 #include <system_error>
 #include <thread>
+#include <utility>
 
 #include <c10/util/Exception.h>
 
@@ -125,7 +126,7 @@ class Lock {
 #ifdef _WIN32
     auto rv = syscall(std::bind(::flock_, fd_, operation));
 #else
-    auto rv = syscall(std::bind(::flock, fd_, operation));
+    auto rv = syscall([this, operation] { return ::flock(fd_, operation); });
 #endif
     SYSASSERT(rv, "flock");
   }
@@ -143,7 +144,9 @@ class File {
       fd_ = syscall(std::bind(
           ::open, path.c_str(), flags | _O_BINARY, _S_IREAD | _S_IWRITE));
 #else
-      fd_ = syscall(std::bind(::open, path.c_str(), flags, 0644));
+      fd_ = syscall([capture0 = path.c_str(), flags] {
+        return ::open(capture0, flags, 0644);
+      });
 #endif
       // Only retry when the file doesn't exist, since we are waiting for the
       // file to be created in this case to address the following issue:
@@ -174,13 +177,14 @@ class File {
   }
 
   off_t seek(off_t offset, int whence) {
-    auto rv = syscall(std::bind(lseek, fd_, offset, whence));
+    auto rv =
+        syscall([this, offset, whence] { return lseek(fd_, offset, whence); });
     SYSASSERT(rv, "lseek");
     return rv;
   }
 
   off_t tell() {
-    auto rv = syscall(std::bind(lseek, fd_, 0, SEEK_CUR));
+    auto rv = syscall([this] { return lseek(fd_, 0, SEEK_CUR); });
     SYSASSERT(rv, "lseek");
     return rv;
   }
@@ -194,7 +198,8 @@ class File {
 
   void write(const void* buf, size_t count) {
     while (count > 0) {
-      auto rv = syscall(std::bind(::write, fd_, buf, count));
+      auto rv =
+          syscall([this, buf, count] { return ::write(fd_, buf, count); });
       SYSASSERT(rv, "write");
       buf = (uint8_t*)buf + rv;
       count -= rv;
@@ -203,7 +208,7 @@ class File {
 
   void read(void* buf, size_t count) {
     while (count > 0) {
-      auto rv = syscall(std::bind(::read, fd_, buf, count));
+      auto rv = syscall([this, buf, count] { return ::read(fd_, buf, count); });
       SYSASSERT(rv, "read");
       buf = (uint8_t*)buf + rv;
       count -= rv;
@@ -225,15 +230,15 @@ class File {
   }
 
   void read(std::string& str) {
-    uint32_t len;
+    uint32_t len = 0;
     read(&len, sizeof(len));
     std::vector<uint8_t> buf(len);
     read(buf.data(), len);
     str.assign(buf.begin(), buf.end());
   }
 
   void read(std::vector<uint8_t>& data) {
-    uint32_t len;
+    uint32_t len = 0;
     read(&len, sizeof(len));
     data.resize(len);
     read(data.data(), len);
@@ -270,9 +275,9 @@ off_t refresh(
 
 } // namespace
 
-FileStore::FileStore(const std::string& path, int numWorkers)
+FileStore::FileStore(std::string path, int numWorkers)
     : Store(),
-      path_(path),
+      path_(std::move(path)),
       pos_(0),
       numWorkers_(numWorkers),
       cleanupKey_("cleanup/"),

diff --git a/torch/csrc/distributed/c10d/FileStore.hpp b/torch/csrc/distributed/c10d/FileStore.hpp
@@ -11,7 +11,7 @@ namespace c10d {
 
 class TORCH_API FileStore : public Store {
  public:
-  explicit FileStore(const std::string& path, int numWorkers);
+  explicit FileStore(std::string  path, int numWorkers);
 
   virtual ~FileStore();
 

diff --git a/torch/csrc/distributed/c10d/GlooDeviceFactory.cpp b/torch/csrc/distributed/c10d/GlooDeviceFactory.cpp
@@ -2,7 +2,7 @@
 
 #ifdef USE_C10D_GLOO
 
-#include <stdlib.h>
+#include <cstdlib>
 
 #include <c10/util/Exception.h>
 

diff --git a/torch/csrc/distributed/c10d/HashStore.cpp b/torch/csrc/distributed/c10d/HashStore.cpp
@@ -1,8 +1,8 @@
 #include <torch/csrc/distributed/c10d/HashStore.hpp>
 
-#include <errno.h>
-#include <stdint.h>
 #include <unistd.h>
+#include <cerrno>
+#include <cstdint>
 
 #include <chrono>
 #include <cstdio>

diff --git a/torch/csrc/distributed/c10d/PrefixStore.cpp b/torch/csrc/distributed/c10d/PrefixStore.cpp
@@ -1,11 +1,10 @@
 #include <torch/csrc/distributed/c10d/PrefixStore.hpp>
+#include <utility>
 
 namespace c10d {
 
-PrefixStore::PrefixStore(
-    const std::string& prefix,
-    c10::intrusive_ptr<Store> store)
-    : prefix_(prefix), store_(store) {}
+PrefixStore::PrefixStore(std::string prefix, c10::intrusive_ptr<Store> store)
+    : prefix_(std::move(prefix)), store_(std::move(store)) {}
 
 std::string PrefixStore::joinKey(const std::string& key) {
   return prefix_ + "/" + key;

diff --git a/torch/csrc/distributed/c10d/PrefixStore.hpp b/torch/csrc/distributed/c10d/PrefixStore.hpp
@@ -8,7 +8,7 @@ namespace c10d {
 class TORCH_API PrefixStore : public Store {
  public:
   explicit PrefixStore(
-      const std::string& prefix,
+      std::string  prefix,
       c10::intrusive_ptr<Store> store);
 
   virtual ~PrefixStore(){};

diff --git a/torch/csrc/distributed/c10d/ProcessGroup.cpp b/torch/csrc/distributed/c10d/ProcessGroup.cpp
@@ -147,7 +147,7 @@ ProcessGroup::ProcessGroup(
 ProcessGroup::ProcessGroup(int rank, int size)
     : rank_(rank), size_(size), backendType_(BackendType::UNDEFINED) {}
 
-ProcessGroup::~ProcessGroup() {}
+ProcessGroup::~ProcessGroup() = default;
 
 void ProcessGroup::init() {
   C10_LOG_API_USAGE_ONCE(

diff --git a/torch/csrc/distributed/c10d/ProcessGroupGloo.cpp b/torch/csrc/distributed/c10d/ProcessGroupGloo.cpp
@@ -626,16 +626,16 @@ void socketInitialize() {
 // gracefully fall back to an alternative if it doesn't.
 bool doesHostnameResolveToUsableAddress(const std::string& hostname) {
   socketInitialize();
-  struct addrinfo hints;
+  struct addrinfo hints {};
   memset(&hints, 0, sizeof(hints));
   hints.ai_family = AF_UNSPEC;
   hints.ai_socktype = SOCK_STREAM;
-  struct addrinfo* result;
+  struct addrinfo* result = nullptr;
   auto rv = getaddrinfo(hostname.c_str(), nullptr, &hints, &result);
   if (rv < 0) {
     return false;
   }
-  struct addrinfo* rp;
+  struct addrinfo* rp = nullptr;
   for (rp = result; rp != nullptr; rp = rp->ai_next) {
     auto fd = socket(rp->ai_family, rp->ai_socktype, rp->ai_protocol);
     if (fd == -1) {

diff --git a/torch/csrc/distributed/c10d/ProcessGroupRoundRobin.cpp b/torch/csrc/distributed/c10d/ProcessGroupRoundRobin.cpp
@@ -18,7 +18,7 @@ ProcessGroupRoundRobin::ProcessGroupRoundRobin(
   iterator_ = processGroups_.begin();
 }
 
-ProcessGroupRoundRobin::~ProcessGroupRoundRobin() {}
+ProcessGroupRoundRobin::~ProcessGroupRoundRobin() = default;
 
 c10::intrusive_ptr<Work> ProcessGroupRoundRobin::broadcast(
     std::vector<at::Tensor>& tensors,