From 3bb20ae49f1b02fb1ff87ac422f54467d94cf304 Mon Sep 17 00:00:00 2001 From: Nikita Shulga Date: Thu, 9 Dec 2021 21:59:50 -0800 Subject: [PATCH] Make c10d tests -Werror clean (#69703) Summary: Pull Request resolved: https://github.com/pytorch/pytorch/pull/69703 Test Plan: Imported from OSS Reviewed By: seemethere Differential Revision: D32997001 Pulled By: malfet fbshipit-source-id: 38b5f195c04f2b3b920e6883a96fe9a36345b9d2 --- test/cpp/c10d/CMakeLists.txt | 1 - test/cpp/c10d/FileStoreTest.cpp | 6 ++-- test/cpp/c10d/HashStoreTest.cpp | 6 ++-- test/cpp/c10d/ProcessGroupGlooAsyncTest.cpp | 6 ++-- test/cpp/c10d/ProcessGroupGlooTest.cpp | 8 ++--- test/cpp/c10d/TCPStoreTest.cpp | 37 +++++++++------------ 6 files changed, 29 insertions(+), 35 deletions(-) diff --git a/test/cpp/c10d/CMakeLists.txt b/test/cpp/c10d/CMakeLists.txt index c640c018038..bf91460c4ba 100644 --- a/test/cpp/c10d/CMakeLists.txt +++ b/test/cpp/c10d/CMakeLists.txt @@ -12,7 +12,6 @@ function(c10d_add_test test_src) target_link_libraries(${test_name} ${ARGN}) if(NOT WIN32) target_link_libraries(${test_name} pthread) - target_compile_options(${test_name} PRIVATE -Wno-error) endif() add_test(NAME ${test_name} COMMAND $) endfunction() diff --git a/test/cpp/c10d/FileStoreTest.cpp b/test/cpp/c10d/FileStoreTest.cpp index 040e15f431b..5e556155c83 100644 --- a/test/cpp/c10d/FileStoreTest.cpp +++ b/test/cpp/c10d/FileStoreTest.cpp @@ -81,14 +81,14 @@ void stressTestStore(std::string path, std::string prefix = "") { std::vector threads; c10d::test::Semaphore sem1, sem2; - for (const auto i : c10::irange(numThreads)) { - threads.push_back(std::thread([&] { + for (C10_UNUSED const auto i : c10::irange(numThreads)) { + threads.emplace_back(std::thread([&] { auto fileStore = c10::make_intrusive(path, numThreads + 1); c10d::PrefixStore store(prefix, fileStore); sem1.post(); sem2.wait(); - for (const auto j : c10::irange(numIterations)) { + for (C10_UNUSED const auto j : c10::irange(numIterations)) { store.add("counter", 1); } })); diff --git a/test/cpp/c10d/HashStoreTest.cpp b/test/cpp/c10d/HashStoreTest.cpp index 311c5295505..095c5d2caa5 100644 --- a/test/cpp/c10d/HashStoreTest.cpp +++ b/test/cpp/c10d/HashStoreTest.cpp @@ -62,11 +62,11 @@ void stressTestStore(std::string prefix = "") { auto hashStore = c10::make_intrusive(); c10d::PrefixStore store(prefix, hashStore); - for (const auto i : c10::irange(numThreads)) { - threads.push_back(std::thread([&] { + for (C10_UNUSED const auto i : c10::irange(numThreads)) { + threads.emplace_back(std::thread([&] { sem1.post(); sem2.wait(); - for (const auto j : c10::irange(numIterations)) { + for (C10_UNUSED const auto j : c10::irange(numIterations)) { store.add("counter", 1); } })); diff --git a/test/cpp/c10d/ProcessGroupGlooAsyncTest.cpp b/test/cpp/c10d/ProcessGroupGlooAsyncTest.cpp index 180145bc041..3032afdc306 100644 --- a/test/cpp/c10d/ProcessGroupGlooAsyncTest.cpp +++ b/test/cpp/c10d/ProcessGroupGlooAsyncTest.cpp @@ -16,12 +16,12 @@ using c10d::ProcessGroup; template std::vector initialize(const std::string& path, int N, Args&&... args) { std::vector tests; - for (const auto i : c10::irange(N)) { + for (C10_UNUSED const auto i : c10::irange(N)) { tests.push_back(std::move(T(path, std::forward(args)...))); } std::vector threads; - for (const auto i : c10::irange(N)) { + for (C10_UNUSED const auto i : c10::irange(N)) { threads.push_back(std::thread([i, N, &tests] { tests[i].start(i, N); })); } @@ -34,7 +34,7 @@ std::vector initialize(const std::string& path, int N, Args&&... args) { class AsyncTest { public: - AsyncTest(const std::string& path) : path_(path) {} + AsyncTest(std::string path) : path_(std::move(path)) {} AsyncTest(AsyncTest&& other) { path_ = std::move(other.path_); diff --git a/test/cpp/c10d/ProcessGroupGlooTest.cpp b/test/cpp/c10d/ProcessGroupGlooTest.cpp index b278bfd3518..88ac369af13 100644 --- a/test/cpp/c10d/ProcessGroupGlooTest.cpp +++ b/test/cpp/c10d/ProcessGroupGlooTest.cpp @@ -126,13 +126,13 @@ class CollectiveTest { int num, bool delayed = false) { std::vector tests; - for (const auto i : c10::irange(num)) { - tests.push_back(CollectiveTest(path)); + for (C10_UNUSED const auto i : c10::irange(num)) { + tests.emplace_back(CollectiveTest(path)); } std::vector threads; for (const auto i : c10::irange(num)) { - threads.push_back(std::thread( + threads.emplace_back(std::thread( [i, &tests, delayed] { tests[i].start(i, tests.size(), delayed); })); } for (auto& thread : threads) { @@ -142,7 +142,7 @@ class CollectiveTest { return tests; } - CollectiveTest(const std::string& path) : path_(path) {} + CollectiveTest(std::string path) : path_(std::move(path)) {} CollectiveTest(CollectiveTest&& other) { path_ = std::move(other.path_); diff --git a/test/cpp/c10d/TCPStoreTest.cpp b/test/cpp/c10d/TCPStoreTest.cpp index d92e245460a..f01423fcb4b 100644 --- a/test/cpp/c10d/TCPStoreTest.cpp +++ b/test/cpp/c10d/TCPStoreTest.cpp @@ -31,8 +31,8 @@ c10::intrusive_ptr _createServer( // Different ports for different tests. void testHelper(const std::string& prefix = "") { - const auto numThreads = 16; - const auto numWorkers = numThreads + 1; + constexpr auto numThreads = 16; + constexpr auto numWorkers = numThreads + 1; auto serverTCPStore = _createServer(numWorkers); @@ -79,7 +79,7 @@ void testHelper(const std::string& prefix = "") { // Hammer on TCPStore std::vector threads; - const auto numIterations = 1000; + constexpr auto numIterations = 1000; c10d::test::Semaphore sem1, sem2; c10d::TCPStoreOptions opts{}; @@ -100,14 +100,12 @@ void testHelper(const std::string& prefix = "") { std::to_string(numThreads * numIterations + 1); for (const auto i : c10::irange(numThreads)) { - threads.emplace_back(std::thread([&sem1, + threads.emplace_back(std::thread([=, + &sem1, &sem2, &clientStores, - i, - &expectedCounterRes, - &numIterations, - &numThreads] { - for (const auto j : c10::irange(numIterations)) { + &expectedCounterRes] { + for (C10_UNUSED const auto j : c10::irange(numIterations)) { clientStores[i]->add("counter", 1); } // Let each thread set and get key on its client store @@ -163,13 +161,12 @@ void testWatchKeyCallback(const std::string& prefix = "") { // were run std::promise numCallbacksExecutedPromise; std::atomic numCallbacksExecuted{0}; - const int numThreads = 16; - const int keyChangeOperation = 3; + constexpr int numThreads = 16; + constexpr int keyChangeOperation = 3; c10d::WatchKeyCallback callback = - [&numCallbacksExecuted, - &numCallbacksExecutedPromise, - &numThreads, - &keyChangeOperation]( + [=, + &numCallbacksExecuted, + &numCallbacksExecutedPromise]( c10::optional /* unused */, c10::optional /* unused */) { numCallbacksExecuted++; @@ -210,12 +207,11 @@ void testWatchKeyCallback(const std::string& prefix = "") { std::vector threads; std::atomic keyChangeOperationCount{0}; for (const auto i : c10::irange(numThreads)) { - threads.emplace_back(std::thread([&clientStores, + threads.emplace_back(std::thread([=, + &clientStores, &internalKey, &internalKeyCount, - &keyChangeOperationCount, - &keyChangeOperation, - i] { + &keyChangeOperationCount] { // Let each thread set and get key on its client store std::string key = internalKey + std::to_string(i); std::string keyCounter = internalKeyCount + std::to_string(i); @@ -275,8 +271,7 @@ void testKeyChangeHelper( c10d::WatchKeyCallback callback = [expectedOldValue, expectedNewValue, &callbackPromise, - &eptr, - &key]( + &eptr]( c10::optional oldValue, c10::optional newValue) { try {