From b473eac5bc1e7209147351058a65c0c1aea5272c Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Tue, 21 May 2024 01:29:07 +0200 Subject: [PATCH] Fix clang-tidy issues in `//common`. (#3962) These likely predate the CI integration for `clang-tidy` runs. Most of these seem good generally, even though I disabled some with nolint comments. The multilevel pointer one seems almost like a bug in the check to detect the specific case of `memcpy`, but otherwise seems like a solid lint. --- common/command_line.h | 6 +++--- common/command_line_test.cpp | 4 ++-- common/hashing.h | 7 +++---- common/hashing_benchmark.cpp | 13 +++++++++++-- common/hashing_test.cpp | 8 ++++++-- 5 files changed, 25 insertions(+), 13 deletions(-) diff --git a/common/command_line.h b/common/command_line.h index 29667214ba73..c8a8a98598f9 100644 --- a/common/command_line.h +++ b/common/command_line.h @@ -223,7 +223,7 @@ class CommandBuilder; // The result of parsing arguments can be a parse error, a successfully parsed // command line, or a meta-success due to triggering a meta-action during the // parse such as rendering help text. -enum class ParseResult { +enum class ParseResult : int8_t { // Signifies an error parsing arguments. It will have been diagnosed using // the streams provided to the parser, and no useful parsed arguments are // available. @@ -285,7 +285,7 @@ struct ArgInfo { }; // The kinds of arguments that can be parsed. -enum class ArgKind { +enum class ArgKind : int8_t { Invalid, Flag, Integer, @@ -578,7 +578,7 @@ struct CommandInfo { // // Commands with _meta_ actions are also a separate kind from those with // normal actions. -enum class CommandKind { +enum class CommandKind : int8_t { Invalid, RequiresSubcommand, Action, diff --git a/common/command_line_test.cpp b/common/command_line_test.cpp index ff0e4115f5f5..d0ee8f867da9 100644 --- a/common/command_line_test.cpp +++ b/common/command_line_test.cpp @@ -26,12 +26,12 @@ constexpr CommandInfo TestCommandInfo = { .help_epilogue = "TODO", }; -enum class TestEnum { +enum class TestEnum : int8_t { Val1, Val2, }; -enum class TestSubcommand { +enum class TestSubcommand : int8_t { Sub1, Sub2, }; diff --git a/common/hashing.h b/common/hashing.h index 450084583e94..9a37b91a5d53 100644 --- a/common/hashing.h +++ b/common/hashing.h @@ -385,7 +385,7 @@ class Hasher { // | sed -e "s/.\{4\}/&'/g" \ // | sed -e "s/\(.\{4\}'.\{4\}'.\{4\}'.\{4\}\)'/0x\1,\n/g" // ``` - static inline constexpr std::array StaticRandomData = { + static constexpr std::array StaticRandomData = { 0x243f'6a88'85a3'08d3, 0x1319'8a2e'0370'7344, 0xa409'3822'299f'31d0, 0x082e'fa98'ec4e'6c89, 0x4528'21e6'38d0'1377, 0xbe54'66cf'34e9'0c6c, 0xc0ac'29b7'c97c'50dd, 0x3f84'd5b5'b547'0917, @@ -558,11 +558,10 @@ inline auto HashValue(const T& value) -> HashCode { return HashValue(value, Hasher::StaticRandomData[7]); } -inline constexpr auto HashCode::ExtractIndex() -> ssize_t { return value_; } +constexpr auto HashCode::ExtractIndex() -> ssize_t { return value_; } template -inline constexpr auto HashCode::ExtractIndexAndTag() - -> std::pair { +constexpr auto HashCode::ExtractIndexAndTag() -> std::pair { static_assert(N >= 1); static_assert(N <= 32); return {static_cast(value_ >> N), diff --git a/common/hashing_benchmark.cpp b/common/hashing_benchmark.cpp index c6a7e3db07e7..b4392fe01f71 100644 --- a/common/hashing_benchmark.cpp +++ b/common/hashing_benchmark.cpp @@ -6,6 +6,7 @@ #include #include +#include #include "absl/hash/hash.h" #include "absl/random/random.h" @@ -54,8 +55,7 @@ static const std::array rand_sizes = []() { // an example of why this is an effective strategy for selecting sizes in the // range. static_assert(NumSizes > 128); - constexpr double Phi = 1.61803398875; - constexpr size_t Scale = std::max(1, MaxSize / Phi); + constexpr size_t Scale = std::max(1, MaxSize / std::numbers::phi); for (auto [i, size] : llvm::enumerate(sizes)) { size = (i * Scale) % MaxSize; } @@ -92,6 +92,10 @@ struct RandValues { static_assert(sizeof(T) <= EntropyObjSize); bytes += sizeof(T); T result; + // Clang Tidy complains about this `memcpy` despite this being the canonical + // formulation. Removing the type `T` would also remove warnings for getting + // the size incorrect. + // NOLINTNEXTLINE(bugprone-multi-level-implicit-pointer-conversion) memcpy(&result, &entropy_bytes[x % EntropySize], sizeof(T)); return result; } @@ -107,7 +111,12 @@ struct RandValues> { bytes += sizeof(std::pair); T result0; U result1; + // Clang Tidy complains about this `memcpy` despite this being the canonical + // formulation. Removing the type `T` would also remove warnings for getting + // the size incorrect. + // NOLINTNEXTLINE(bugprone-multi-level-implicit-pointer-conversion) memcpy(&result0, &entropy_bytes[x % EntropySize], sizeof(T)); + // NOLINTNEXTLINE(bugprone-multi-level-implicit-pointer-conversion) memcpy(&result1, &entropy_bytes[x % EntropySize] + sizeof(T), sizeof(U)); return {result0, result1}; } diff --git a/common/hashing_test.cpp b/common/hashing_test.cpp index 65c2ab44b2a9..473b163be481 100644 --- a/common/hashing_test.cpp +++ b/common/hashing_test.cpp @@ -74,14 +74,14 @@ TEST(HashingTest, Integers) { EXPECT_THAT(hash, Ne(hash_zero)); } }; - test_int_hash(i); test_int_hash(static_cast(i)); test_int_hash(static_cast(i)); test_int_hash(static_cast(i)); test_int_hash(static_cast(i)); test_int_hash(static_cast(i)); test_int_hash(static_cast(i)); - test_int_hash(static_cast(i)); + // `i` is already an int64_t variable. + test_int_hash(i); test_int_hash(static_cast(i)); } } @@ -332,11 +332,15 @@ template auto PrintFullWidthHex(llvm::raw_ostream& os, T value) { static_assert(sizeof(T) == 1 || sizeof(T) == 2 || sizeof(T) == 4 || sizeof(T) == 8); + // Given the nature of a format string and the good formatting, a nested + // conditional seems like the most readable structure. + // NOLINTBEGIN(readability-avoid-nested-conditional-operator) os << llvm::formatv(sizeof(T) == 1 ? "{0:x2}" : sizeof(T) == 2 ? "{0:x4}" : sizeof(T) == 4 ? "{0:x8}" : "{0:x16}", static_cast(value)); + // NOLINTEND(readability-avoid-nested-conditional-operator) } template