From ad507789d12e3f757a43f35cd66e7aea6124140d Mon Sep 17 00:00:00 2001 From: cyy Date: Mon, 8 Jan 2024 11:07:55 +0000 Subject: [PATCH] [Reland] [11/N] Enable clang-tidy warnings on c10/util/*.h (#116751) Reland of #116353 with C++ diagnostic macros restored. Pull Request resolved: https://github.com/pytorch/pytorch/pull/116751 Approved by: https://github.com/albanD --- .lintrunner.toml | 11 ++++++ c10/util/ApproximateClock.h | 6 +-- c10/util/ArrayRef.h | 3 ++ c10/util/ConstexprCrc.h | 1 + c10/util/ExclusivelyOwned.h | 5 +-- c10/util/Half.h | 2 - c10/util/IdWrapper.h | 1 - c10/util/MaybeOwned.h | 3 ++ c10/util/Optional.h | 7 +++- c10/util/OptionalArrayRef.h | 12 +++--- c10/util/ScopeExit.h | 4 +- c10/util/StringUtil.h | 6 ++- c10/util/TypeCast.h | 1 + c10/util/accumulate.h | 6 ++- c10/util/flat_hash_map.h | 45 ++++++++--------------- c10/util/intrusive_ptr.h | 3 +- c10/util/llvmMathExtras.h | 8 +++- c10/util/order_preserving_flat_hash_map.h | 34 +++++------------ c10/util/signal_handler.h | 1 + c10/util/string_utils.h | 5 +++ torch/csrc/lazy/core/ir_metadata.h | 2 +- 21 files changed, 80 insertions(+), 86 deletions(-) diff --git a/.lintrunner.toml b/.lintrunner.toml index 4e862f43798..c1f4cb6b5b9 100644 --- a/.lintrunner.toml +++ b/.lintrunner.toml @@ -245,6 +245,7 @@ include_patterns = [ 'aten/src/ATen/core/*.cpp', 'c10/**/*.cpp', 'c10/core/**/*.h', + 'c10/util/**/*.h', 'torch/csrc/**/*.cpp', ] exclude_patterns = [ @@ -255,6 +256,16 @@ exclude_patterns = [ '**/*pb.h', '**/*CUDA*', '**/cuda/*pp', + 'c10/util/complex_math.h', + 'c10/util/complex_utils.h', + 'c10/util/flat_hash_map.h', + 'c10/util/Float8*.h', + 'c10/util/logging*.h', + 'c10/util/hash.h', + 'c10/util/strong_type.h', + 'c10/util/SmallVector.h', + 'c10/util/win32-headers.h', + 'c10/util/*inl.h', 'aten/src/ATen/core/TensorImpl_test.cpp', 'third_party/**/*', 'torch/csrc/api/**', diff --git a/c10/util/ApproximateClock.h b/c10/util/ApproximateClock.h index 95b426c4a85..4b3b5b6bb9a 100644 --- a/c10/util/ApproximateClock.h +++ b/c10/util/ApproximateClock.h @@ -7,14 +7,10 @@ #include #include #include +#include #include #include -#include - -#ifndef _WIN32 -#include -#endif #if defined(C10_IOS) && defined(C10_MOBILE) #include // for gettimeofday() #endif diff --git a/c10/util/ArrayRef.h b/c10/util/ArrayRef.h index 907fe408e7b..59ea43f8d95 100644 --- a/c10/util/ArrayRef.h +++ b/c10/util/ArrayRef.h @@ -125,6 +125,7 @@ class ArrayRef final { /// Construct an ArrayRef from a C array. template + // NOLINTNEXTLINE(*c-arrays*) /* implicit */ constexpr ArrayRef(const T (&Arr)[N]) : Data(Arr), Length(N) {} /// Construct an ArrayRef from a std::initializer_list. @@ -238,6 +239,7 @@ class ArrayRef final { /// continues to select the move assignment operator. template std::enable_if_t, ArrayRef>& operator=( + // NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward) U&& Temporary) = delete; /// Disallow accidental assignment from a temporary. @@ -330,6 +332,7 @@ ArrayRef& makeArrayRef(ArrayRef& Vec) { /// Construct an ArrayRef from a C array. template +// NOLINTNEXTLINE(*c-arrays*) ArrayRef makeArrayRef(const T (&Arr)[N]) { return ArrayRef(Arr); } diff --git a/c10/util/ConstexprCrc.h b/c10/util/ConstexprCrc.h index 65c4edbc4a7..0eec44d576e 100644 --- a/c10/util/ConstexprCrc.h +++ b/c10/util/ConstexprCrc.h @@ -8,6 +8,7 @@ namespace c10::util { namespace detail { +// NOLINTNEXTLINE(*c-arrays*) constexpr uint64_t crc64_table[] = { 0x0000000000000000, 0x7ad870c830358979, 0xf5b0e190606b12f2, 0x8f689158505e9b8b, 0xc038e5739841b68f, 0xbae095bba8743ff6, diff --git a/c10/util/ExclusivelyOwned.h b/c10/util/ExclusivelyOwned.h index c89cb501d45..c2ff416380c 100644 --- a/c10/util/ExclusivelyOwned.h +++ b/c10/util/ExclusivelyOwned.h @@ -55,10 +55,7 @@ struct ExclusivelyOwnedTraits; template class ExclusivelyOwned { using EOT = ExclusivelyOwnedTraits; - union { - char dummy_; - typename ExclusivelyOwnedTraits::repr_type repr_; - }; + typename ExclusivelyOwnedTraits::repr_type repr_; public: ExclusivelyOwned() : repr_(EOT::nullRepr()) {} diff --git a/c10/util/Half.h b/c10/util/Half.h index 3c5f4a15107..f9a053871db 100644 --- a/c10/util/Half.h +++ b/c10/util/Half.h @@ -18,10 +18,8 @@ #if defined(__cplusplus) && (__cplusplus >= 201103L) #include -#include #elif !defined(__OPENCL_VERSION__) #include -#include #endif #ifdef _MSC_VER diff --git a/c10/util/IdWrapper.h b/c10/util/IdWrapper.h index 59b5088c270..1426ee9362a 100644 --- a/c10/util/IdWrapper.h +++ b/c10/util/IdWrapper.h @@ -1,6 +1,5 @@ #pragma once -#include #include #include #include diff --git a/c10/util/MaybeOwned.h b/c10/util/MaybeOwned.h index 98a4e870166..41f6d2db4ac 100644 --- a/c10/util/MaybeOwned.h +++ b/c10/util/MaybeOwned.h @@ -126,6 +126,7 @@ class MaybeOwned final { } MaybeOwned(MaybeOwned&& rhs) noexcept( + // NOLINTNEXTLINE(*-noexcept-move-*) std::is_nothrow_move_constructible_v && std::is_nothrow_move_assignable_v) : isBorrowed_(rhs.isBorrowed_) { @@ -140,6 +141,7 @@ class MaybeOwned final { std::is_nothrow_move_assignable_v && std::is_nothrow_move_assignable_v && std::is_nothrow_move_constructible_v && + // NOLINTNEXTLINE(*-noexcept-move-*) std::is_nothrow_destructible_v && std::is_nothrow_destructible_v) { if (this == &rhs) { @@ -180,6 +182,7 @@ class MaybeOwned final { } ~MaybeOwned() noexcept( + // NOLINTNEXTLINE(*-noexcept-destructor) std::is_nothrow_destructible_v && std::is_nothrow_destructible_v) { if (C10_UNLIKELY(!isBorrowed_)) { diff --git a/c10/util/Optional.h b/c10/util/Optional.h index 9d45e01f161..70fa29e6f39 100644 --- a/c10/util/Optional.h +++ b/c10/util/Optional.h @@ -7,14 +7,17 @@ // Macros.h is not needed, but it does namespace shenanigans that lots // of downstream code seems to rely on. Feel free to remove it and fix // up builds. -#include -#include namespace c10 { +// NOLINTNEXTLINE(misc-unused-using-decls) using std::bad_optional_access; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::make_optional; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::nullopt; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::nullopt_t; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::optional; namespace detail_ { diff --git a/c10/util/OptionalArrayRef.h b/c10/util/OptionalArrayRef.h index 7e98da59f08..2c2b88722d4 100644 --- a/c10/util/OptionalArrayRef.h +++ b/c10/util/OptionalArrayRef.h @@ -110,13 +110,13 @@ class OptionalArrayRef final { return *this; } - template > - constexpr std::enable_if_t< - !std::is_same_v, OptionalArrayRef> && + template < + typename U = ArrayRef, + typename = std::enable_if_t< + !std::is_same_v, OptionalArrayRef> && std::is_constructible_v, U&&> && - std::is_assignable_v&, U&&>, - OptionalArrayRef&> - operator=(U&& value) noexcept( + std::is_assignable_v&, U&&>>> + constexpr OptionalArrayRef& operator=(U&& value) noexcept( std::is_nothrow_constructible_v, U&&> && std::is_nothrow_assignable_v&, U&&>) { wrapped_opt_array_ref = std::forward(value); diff --git a/c10/util/ScopeExit.h b/c10/util/ScopeExit.h index 85807867c09..d1e7e1ad547 100644 --- a/c10/util/ScopeExit.h +++ b/c10/util/ScopeExit.h @@ -15,9 +15,7 @@ class scope_exit { public: template - // constructor accepting a forwarding reference can hide the - // move constructor - // @lint-ignore CLANGTIDY + // NOLINTNEXTLINE(bugprone-forwarding-reference-overload) explicit scope_exit(Fp&& F) : ExitFunction(std::forward(F)) {} scope_exit(scope_exit&& Rhs) noexcept diff --git a/c10/util/StringUtil.h b/c10/util/StringUtil.h index 6a29c407cb9..0d7b0303a81 100644 --- a/c10/util/StringUtil.h +++ b/c10/util/StringUtil.h @@ -9,7 +9,6 @@ #include #include #include -#include C10_CLANG_DIAGNOSTIC_PUSH() #if C10_CLANG_HAS_WARNING("-Wshorten-64-to-32") @@ -41,6 +40,7 @@ struct CanonicalizeStrTypes { }; template +// NOLINTNEXTLINE(*c-arrays*) struct CanonicalizeStrTypes { using type = const char*; }; @@ -181,11 +181,15 @@ inline void printQuotedString(std::ostream& stmt, const string_view str) { } else { // C++ io has stateful formatting settings. Messing with // them is probably worse than doing this manually. + // NOLINTNEXTLINE(*c-arrays*) char buf[4] = "000"; + // NOLINTNEXTLINE(*narrowing-conversions) buf[2] += s % 8; s /= 8; + // NOLINTNEXTLINE(*narrowing-conversions) buf[1] += s % 8; s /= 8; + // NOLINTNEXTLINE(*narrowing-conversions) buf[0] += s; stmt << "\\" << buf; } diff --git a/c10/util/TypeCast.h b/c10/util/TypeCast.h index f28beda8987..31fe3a3397a 100644 --- a/c10/util/TypeCast.h +++ b/c10/util/TypeCast.h @@ -6,6 +6,7 @@ #include #include #include +#include #include diff --git a/c10/util/accumulate.h b/c10/util/accumulate.h index 961497a3290..9d7b31f7aaa 100644 --- a/c10/util/accumulate.h +++ b/c10/util/accumulate.h @@ -2,11 +2,13 @@ #pragma once -#include - +#include +#include +#include #include #include #include +#include namespace c10 { diff --git a/c10/util/flat_hash_map.h b/c10/util/flat_hash_map.h index 84f12ba015b..8688510b2b8 100644 --- a/c10/util/flat_hash_map.h +++ b/c10/util/flat_hash_map.h @@ -8,6 +8,7 @@ // - make sherwood_v3_table::convertible_to_iterator public because GCC5 seems // to have issues with it otherwise // - fix compiler warnings in operator templated_iterator +// - make use of 'if constexpr' and eliminate AssignIfTrue template // Copyright Malte Skarupke 2017. // Distributed under the Boost Software License, Version 1.0. @@ -176,6 +177,7 @@ struct sherwood_v3_entry { }; inline int8_t log2(uint64_t value) { + // NOLINTNEXTLINE(*c-arrays*) static constexpr int8_t table[64] = { 63, 0, 58, 1, 59, 47, 53, 2, 60, 39, 48, 27, 54, 33, 42, 3, 61, 51, 37, 40, 49, 18, 28, 20, 55, 30, 34, 11, 43, 14, 22, 4, @@ -190,21 +192,6 @@ inline int8_t log2(uint64_t value) { return table[((value - (value >> 1)) * 0x07EDD5E59A4E28C2) >> 58]; } -template -struct AssignIfTrue { - void operator()(T& lhs, const T& rhs) { - lhs = rhs; - } - void operator()(T& lhs, T&& rhs) { - lhs = std::move(rhs); - } -}; -template -struct AssignIfTrue { - void operator()(T&, const T&) {} - void operator()(T&, T&&) {} -}; - inline uint64_t next_power_of_two(uint64_t i) { --i; i |= i >> 1; @@ -389,15 +376,13 @@ class sherwood_v3_table : private EntryAlloc, return *this; clear(); - if (AllocatorTraits::propagate_on_container_copy_assignment::value) { + if constexpr (AllocatorTraits::propagate_on_container_copy_assignment:: + value) { if (static_cast(*this) != static_cast(other)) { reset_to_empty_state(); } - AssignIfTrue< - EntryAlloc, - AllocatorTraits::propagate_on_container_copy_assignment::value>()( - *this, other); + static_cast(*this) = other; } _max_load_factor = other._max_load_factor; static_cast(*this) = other; @@ -409,13 +394,11 @@ class sherwood_v3_table : private EntryAlloc, sherwood_v3_table& operator=(sherwood_v3_table&& other) noexcept { if (this == std::addressof(other)) return *this; - else if (AllocatorTraits::propagate_on_container_move_assignment::value) { + else if constexpr (AllocatorTraits::propagate_on_container_move_assignment:: + value) { clear(); reset_to_empty_state(); - AssignIfTrue< - EntryAlloc, - AllocatorTraits::propagate_on_container_move_assignment::value>()( - *this, std::move(other)); + static_cast(*this) = std::move(other); swap_pointers(other); } else if ( static_cast(*this) == static_cast(other)) { @@ -543,6 +526,7 @@ class sherwood_v3_table : private EntryAlloc, return end(); } const_iterator find(const FindKey& key) const { + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) return const_cast(this)->find(key); } uint64_t count(const FindKey& key) const { @@ -795,7 +779,8 @@ class sherwood_v3_table : private EntryAlloc, uint64_t num_buckets_for_reserve(uint64_t num_elements_) const { return static_cast(std::ceil( - num_elements_ / std::min(0.5, static_cast(_max_load_factor)))); + static_cast(num_elements_) / + std::min(0.5, static_cast(_max_load_factor)))); } void rehash_for_other_container(const sherwood_v3_table& other) { rehash( @@ -1486,6 +1471,7 @@ struct prime_number_hash_policy { // ClosestPrime(p * 2^(1/3)) and ClosestPrime(p * 2^(2/3)) and put those in // the gaps // 5. get PrevPrime(2^64) and put it at the end + // NOLINTNEXTLINE(*c-arrays*) static constexpr const uint64_t prime_list[] = { 2llu, 3llu, @@ -1673,6 +1659,7 @@ struct prime_number_hash_policy { 11493228998133068689llu, 14480561146010017169llu, 18446744073709551557llu}; + // NOLINTNEXTLINE(*c-arrays*) static constexpr uint64_t (*const mod_functions[])(uint64_t) = { &mod0, &mod2, @@ -1911,7 +1898,7 @@ struct fibonacci_hash_policy { int8_t next_size_over(uint64_t& size) const { size = std::max(uint64_t(2), detailv3::next_power_of_two(size)); - return 64 - detailv3::log2(size); + return static_cast(64 - detailv3::log2(size)); } void commit(int8_t shift_) { shift = shift_; @@ -2020,9 +2007,7 @@ class flat_hash_map return false; for (const typename Table::value_type& value : lhs) { auto found = rhs.find(value.first); - if (found == rhs.end()) - return false; - else if (value.second != found->second) + if (found == rhs.end() || value.second != found->second) return false; } return true; diff --git a/c10/util/intrusive_ptr.h b/c10/util/intrusive_ptr.h index 22815609c3d..da6865659e6 100644 --- a/c10/util/intrusive_ptr.h +++ b/c10/util/intrusive_ptr.h @@ -388,8 +388,7 @@ class intrusive_ptr final { if (this == &rhs) { return *this; } - // NOLINTNEXTLINE(misc-unconventional-assign-operator, - // cppcoreguidelines-c-copy-assignment-signature) + // NOLINTNEXTLINE(*assign-operator, *assignment-signature) return operator= (rhs); } diff --git a/c10/util/llvmMathExtras.h b/c10/util/llvmMathExtras.h index 4904369a89b..82651f7858c 100644 --- a/c10/util/llvmMathExtras.h +++ b/c10/util/llvmMathExtras.h @@ -283,7 +283,8 @@ T findLastSet(T Val, ZeroBehavior ZB = ZB_Max) { /// Macro compressed bit reversal table for 256 bits. /// /// http://graphics.stanford.edu/~seander/bithacks.html#BitReverseTable -static const unsigned char BitReverseTable256[256] = { +/// NOLINTNEXTLINE(*c-arrays*) +static constexpr unsigned char BitReverseTable256[256] = { #define R2(n) n, n + 2 * 64, n + 1 * 64, n + 3 * 64 #define R4(n) R2(n), R2(n + 2 * 16), R2(n + 1 * 16), R2(n + 3 * 16) #define R6(n) R4(n), R4(n + 2 * 4), R4(n + 1 * 4), R4(n + 3 * 4) @@ -299,7 +300,9 @@ static const unsigned char BitReverseTable256[256] = { /// Reverse the bits in \p Val. template T reverseBits(T Val) { + // NOLINTNEXTLINE(*c-arrays*) unsigned char in[sizeof(Val)]; + // NOLINTNEXTLINE(*c-arrays*) unsigned char out[sizeof(Val)]; std::memcpy(in, &Val, sizeof(Val)); for (unsigned i = 0; i < sizeof(Val); ++i) @@ -420,7 +423,7 @@ inline uint64_t maxUIntN(uint64_t N) { /// Gets the minimum value for a N-bit signed integer. inline int64_t minIntN(int64_t N) { assert(N > 0 && N <= 64 && "integer width out of range"); - + // NOLINTNEXTLINE(*-narrowing-conversions) return -(UINT64_C(1) << (N - 1)); } @@ -434,6 +437,7 @@ inline int64_t maxIntN(int64_t N) { // This relies on two's complement wraparound when N == 64, so we convert to // int64_t only at the very end to avoid UB. + // NOLINTNEXTLINE(*-narrowing-conversions) return (UINT64_C(1) << (N - 1)) - 1; } diff --git a/c10/util/order_preserving_flat_hash_map.h b/c10/util/order_preserving_flat_hash_map.h index 72a7e488b6f..02199560034 100644 --- a/c10/util/order_preserving_flat_hash_map.h +++ b/c10/util/order_preserving_flat_hash_map.h @@ -8,6 +8,7 @@ // - make sherwood_v3_table::convertible_to_iterator public because GCC5 seems // to have issues with it otherwise // - fix compiler warnings in operator templated_iterator +// - make use of 'if constexpr' and eliminate AssignIfTrue template // Copyright Malte Skarupke 2017. // Distributed under the Boost Software License, Version 1.0. @@ -139,9 +140,11 @@ struct KeyOrValueEquality : functor_storage { static constexpr int8_t min_lookups = 4; template struct sherwood_v3_entry { + // NOLINTNEXTLINE(modernize-use-equals-default) sherwood_v3_entry() {} sherwood_v3_entry(int8_t distance_from_desired) : distance_from_desired(distance_from_desired) {} + // NOLINTNEXTLINE(modernize-use-equals-default) ~sherwood_v3_entry() {} bool has_value() const { @@ -188,21 +191,6 @@ inline int8_t log2(uint64_t value) { return table[((value - (value >> 1)) * 0x07EDD5E59A4E28C2) >> 58]; } -template -struct AssignIfTrue { - void operator()(T& lhs, const T& rhs) { - lhs = rhs; - } - void operator()(T& lhs, T&& rhs) { - lhs = std::move(rhs); - } -}; -template -struct AssignIfTrue { - void operator()(T&, const T&) {} - void operator()(T&, T&&) {} -}; - inline uint64_t next_power_of_two(uint64_t i) { --i; i |= i >> 1; @@ -383,15 +371,13 @@ class sherwood_v3_table : private EntryAlloc, private Hasher, private Equal { return *this; clear(); - if (AllocatorTraits::propagate_on_container_copy_assignment::value) { + if constexpr (AllocatorTraits::propagate_on_container_copy_assignment:: + value) { if (static_cast(*this) != static_cast(other)) { reset_to_empty_state(); } - AssignIfTrue< - EntryAlloc, - AllocatorTraits::propagate_on_container_copy_assignment::value>()( - *this, other); + static_cast(*this) = other; } _max_load_factor = other._max_load_factor; static_cast(*this) = other; @@ -403,13 +389,11 @@ class sherwood_v3_table : private EntryAlloc, private Hasher, private Equal { sherwood_v3_table& operator=(sherwood_v3_table&& other) noexcept { if (this == std::addressof(other)) return *this; - else if (AllocatorTraits::propagate_on_container_move_assignment::value) { + else if constexpr (AllocatorTraits::propagate_on_container_move_assignment:: + value) { clear(); reset_to_empty_state(); - AssignIfTrue< - EntryAlloc, - AllocatorTraits::propagate_on_container_move_assignment::value>()( - *this, std::move(other)); + static_cast(*this) = std::move(other); swap_pointers(other); } else if ( static_cast(*this) == static_cast(other)) { diff --git a/c10/util/signal_handler.h b/c10/util/signal_handler.h index f709de10f5c..3efaf27b85c 100644 --- a/c10/util/signal_handler.h +++ b/c10/util/signal_handler.h @@ -98,6 +98,7 @@ class C10_API FatalSignalHandler { struct sigaction previous; }; + // NOLINTNEXTLINE(*c-arrays*) static signal_handler kSignalHandlers[]; }; diff --git a/c10/util/string_utils.h b/c10/util/string_utils.h index 8541b16980b..92af736452a 100644 --- a/c10/util/string_utils.h +++ b/c10/util/string_utils.h @@ -4,10 +4,15 @@ namespace c10 { +// NOLINTNEXTLINE(misc-unused-using-decls) using std::stod; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::stoi; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::stoll; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::stoull; +// NOLINTNEXTLINE(misc-unused-using-decls) using std::to_string; } // namespace c10 diff --git a/torch/csrc/lazy/core/ir_metadata.h b/torch/csrc/lazy/core/ir_metadata.h index ea413fcfb82..435785df8ff 100644 --- a/torch/csrc/lazy/core/ir_metadata.h +++ b/torch/csrc/lazy/core/ir_metadata.h @@ -1,6 +1,6 @@ #pragma once -#include +#include #include #include