diff options
22 files changed, 94 insertions, 74 deletions
diff --git a/.clang-tidy b/.clang-tidy index e1b14f15..284b57cf 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -11,6 +11,6 @@ WarningsAsErrors: >- modernize-*,-modernize-return-braced-init-list, performance-*,-performance-avoid-endl, portability-*, - readability-redundant-member-init, + readability-*,-readability-function-cognitive-complexity,-readability-identifier-length, CheckOptions: - { key: misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic, value: '1' } diff --git a/src/buildtool/build_engine/base_maps/entity_name.hpp b/src/buildtool/build_engine/base_maps/entity_name.hpp index def2cbe6..ad2b368d 100644 --- a/src/buildtool/build_engine/base_maps/entity_name.hpp +++ b/src/buildtool/build_engine/base_maps/entity_name.hpp @@ -112,14 +112,19 @@ template <typename T> std::optional<std::function<void(std::string const&)>> logger = std::nullopt) noexcept -> std::optional<EntityName> { try { - bool const is_file = s0 == EntityName::kFileLocationMarker; - bool const is_glob = s0 == EntityName::kGlobMarker; - bool const is_symlink = s0 == EntityName::kSymlinkLocationMarker; - auto const ref_type = - is_file ? ReferenceType::kFile - : (is_glob ? ReferenceType::kGlob - : (is_symlink ? ReferenceType::kSymlink - : ReferenceType::kTree)); + auto get_ref_type = [](std::string const& s) -> ReferenceType { + if (s == EntityName::kFileLocationMarker) { + return ReferenceType::kFile; + } + if (s == EntityName::kGlobMarker) { + return ReferenceType::kGlob; + } + if (s == EntityName::kSymlinkLocationMarker) { + return ReferenceType::kSymlink; + } + return ReferenceType::kTree; + }; + auto const ref_type = get_ref_type(s0); if (list_size == 3) { if (IsString(list[2])) { auto const& name = GetString(list[2]); diff --git a/src/buildtool/build_engine/base_maps/user_rule.hpp b/src/buildtool/build_engine/base_maps/user_rule.hpp index 263dd30c..327d1466 100644 --- a/src/buildtool/build_engine/base_maps/user_rule.hpp +++ b/src/buildtool/build_engine/base_maps/user_rule.hpp @@ -372,6 +372,8 @@ static auto inline FindDuplicates(gsl::not_null<T_Result*> const& dups, FindDuplicates</*kTriangular=*/false>(dups, first, rest...); if constexpr (kTriangular) { // do triangular compare of second with rest + + // NOLINTNEXTLINE(readability-suspicious-call-argument) FindDuplicates</*kTriangular=*/true>(dups, second, rest...); } } diff --git a/src/buildtool/build_engine/expression/expression.cpp b/src/buildtool/build_engine/expression/expression.cpp index 7d0f55b5..cbb6a18d 100644 --- a/src/buildtool/build_engine/expression/expression.cpp +++ b/src/buildtool/build_engine/expression/expression.cpp @@ -246,11 +246,19 @@ auto Expression::ComputeHash() const noexcept -> std::string { IsResult() or IsNode() or IsName()) { // just hash the JSON representation, but prepend "@" for artifact, // "=" for result, "#" for node, and "$" for name. - std::string prefix{IsArtifact() ? "@" - : IsResult() ? "=" - : IsNode() ? "#" - : IsName() ? "$" - : ""}; + std::string prefix; + if (IsArtifact()) { + prefix = "@"; + } + else if (IsResult()) { + prefix = "="; + } + else if (IsNode()) { + prefix = "#"; + } + else if (IsName()) { + prefix = "$"; + } hash = hash_function.PlainHashData(prefix + ToString()).Bytes(); } else { diff --git a/src/buildtool/build_engine/target_map/built_in_rules.cpp b/src/buildtool/build_engine/target_map/built_in_rules.cpp index 9ff82faf..572c9abb 100644 --- a/src/buildtool/build_engine/target_map/built_in_rules.cpp +++ b/src/buildtool/build_engine/target_map/built_in_rules.cpp @@ -922,8 +922,8 @@ void InstallRule( std::vector<std::pair<BuildMaps::Base::EntityName, std::string>>{}; dirs.reserve(dirs_value->List().size()); for (auto const& entry : dirs_value->List()) { - if (not(entry->IsList() and entry->List().size() == 2 and - entry->List()[1]->IsString())) { + if (not entry->IsList() or entry->List().size() != 2 or + not entry->List()[1]->IsString()) { (*logger)(fmt::format("Expected dirs to evaluate to a list of " "target-path pairs, but found entry {}", entry->ToString()), diff --git a/src/buildtool/common/protocol_traits.hpp b/src/buildtool/common/protocol_traits.hpp index ce596edb..eca2a807 100644 --- a/src/buildtool/common/protocol_traits.hpp +++ b/src/buildtool/common/protocol_traits.hpp @@ -19,13 +19,13 @@ class ProtocolTraits final { public: - inline static constexpr auto IsNative(HashFunction::Type hash_type) noexcept + static constexpr auto IsNative(HashFunction::Type hash_type) noexcept -> bool { return hash_type == HashFunction::Type::GitSHA1; } - inline static constexpr auto IsTreeAllowed( - HashFunction::Type hash_type) noexcept -> bool { + static constexpr auto IsTreeAllowed(HashFunction::Type hash_type) noexcept + -> bool { return IsNative(hash_type); } }; diff --git a/src/buildtool/common/remote/remote_common.hpp b/src/buildtool/common/remote/remote_common.hpp index 675f41b2..b9bc926a 100644 --- a/src/buildtool/common/remote/remote_common.hpp +++ b/src/buildtool/common/remote/remote_common.hpp @@ -86,7 +86,7 @@ using DispatchEndpoint = std::pair<ExecutionProperties, ServerAddress>; dispatch.dump())}; } for (auto const& entry : dispatch) { - if (not(entry.is_array() and entry.size() == 2)) { + if (not entry.is_array() or entry.size() != 2) { return unexpected{ fmt::format("Endpoint configuration has to be a list of " "pairs, but found entry {}", diff --git a/src/buildtool/crypto/hash_info.hpp b/src/buildtool/crypto/hash_info.hpp index dcfde884..dc40e7ca 100644 --- a/src/buildtool/crypto/hash_info.hpp +++ b/src/buildtool/crypto/hash_info.hpp @@ -65,21 +65,17 @@ class HashInfo final { bool is_tree) noexcept -> std::optional<std::pair<HashInfo, std::uintmax_t>>; - [[nodiscard]] inline auto Hash() const& noexcept -> std::string const& { + [[nodiscard]] auto Hash() const& noexcept -> std::string const& { return hash_; } - [[nodiscard]] inline auto Hash() && -> std::string { - return std::move(hash_); - } + [[nodiscard]] auto Hash() && -> std::string { return std::move(hash_); } - [[nodiscard]] inline auto HashType() const noexcept -> HashFunction::Type { + [[nodiscard]] auto HashType() const noexcept -> HashFunction::Type { return hash_type_; } - [[nodiscard]] inline auto IsTree() const noexcept -> bool { - return is_tree_; - } + [[nodiscard]] auto IsTree() const noexcept -> bool { return is_tree_; } [[nodiscard]] auto operator==(HashInfo const& other) const noexcept -> bool; diff --git a/src/buildtool/crypto/hasher.cpp b/src/buildtool/crypto/hasher.cpp index 41182559..7c5d1d9c 100644 --- a/src/buildtool/crypto/hasher.cpp +++ b/src/buildtool/crypto/hasher.cpp @@ -66,15 +66,15 @@ struct InitializeVisitor final { static constexpr std::string_view kLogInfo = "Initialize"; // NOLINTNEXTLINE(google-runtime-references) - [[nodiscard]] inline auto operator()(SHA_CTX& ctx) const -> bool { + [[nodiscard]] auto operator()(SHA_CTX& ctx) const -> bool { return SHA1_Init(&ctx) == kOpenSslTrue; } // NOLINTNEXTLINE(google-runtime-references) - [[nodiscard]] inline auto operator()(SHA256_CTX& ctx) const -> bool { + [[nodiscard]] auto operator()(SHA256_CTX& ctx) const -> bool { return SHA256_Init(&ctx) == kOpenSslTrue; } // NOLINTNEXTLINE(google-runtime-references) - [[nodiscard]] inline auto operator()(SHA512_CTX& ctx) const -> bool { + [[nodiscard]] auto operator()(SHA512_CTX& ctx) const -> bool { return SHA512_Init(&ctx) == kOpenSslTrue; } }; @@ -86,15 +86,15 @@ struct UpdateVisitor final { : data_{*data} {} // NOLINTNEXTLINE(google-runtime-references) - [[nodiscard]] inline auto operator()(SHA_CTX& ctx) const -> bool { + [[nodiscard]] auto operator()(SHA_CTX& ctx) const -> bool { return SHA1_Update(&ctx, data_.data(), data_.size()) == kOpenSslTrue; } // NOLINTNEXTLINE(google-runtime-references) - [[nodiscard]] inline auto operator()(SHA256_CTX& ctx) const -> bool { + [[nodiscard]] auto operator()(SHA256_CTX& ctx) const -> bool { return SHA256_Update(&ctx, data_.data(), data_.size()) == kOpenSslTrue; } // NOLINTNEXTLINE(google-runtime-references) - [[nodiscard]] inline auto operator()(SHA512_CTX& ctx) const -> bool { + [[nodiscard]] auto operator()(SHA512_CTX& ctx) const -> bool { return SHA512_Update(&ctx, data_.data(), data_.size()) == kOpenSslTrue; } diff --git a/src/buildtool/execution_api/common/tree_reader.hpp b/src/buildtool/execution_api/common/tree_reader.hpp index 20e73d7d..9c1b295e 100644 --- a/src/buildtool/execution_api/common/tree_reader.hpp +++ b/src/buildtool/execution_api/common/tree_reader.hpp @@ -111,7 +111,7 @@ class TreeReader final { private: TImpl impl_; - [[nodiscard]] static inline auto IsDirectoryEmpty( + [[nodiscard]] static auto IsDirectoryEmpty( bazel_re::Directory const& dir) noexcept -> bool { return dir.files().empty() and dir.directories().empty() and dir.symlinks().empty(); diff --git a/src/buildtool/execution_api/local/local_api.hpp b/src/buildtool/execution_api/local/local_api.hpp index e564d72f..7ba26b3d 100644 --- a/src/buildtool/execution_api/local/local_api.hpp +++ b/src/buildtool/execution_api/local/local_api.hpp @@ -263,17 +263,17 @@ class LocalApi final : public IExecutionApi { [[nodiscard]] auto Upload(ArtifactBlobContainer&& blobs, bool /*skip_find_missing*/) const noexcept -> bool final { - for (auto const& blob : blobs.Blobs()) { - auto const cas_digest = - blob.digest.IsTree() - ? local_context_.storage->CAS().StoreTree(*blob.data) - : local_context_.storage->CAS().StoreBlob(*blob.data, - blob.is_exec); - if (not cas_digest or *cas_digest != blob.digest) { - return false; - } - } - return true; + auto const range = blobs.Blobs(); + return std::all_of( + range.begin(), + range.end(), + [&cas = local_context_.storage->CAS()](ArtifactBlob const& blob) { + auto const cas_digest = + blob.digest.IsTree() + ? cas.StoreTree(*blob.data) + : cas.StoreBlob(*blob.data, blob.is_exec); + return cas_digest and *cas_digest == blob.digest; + }); } [[nodiscard]] auto UploadTree( diff --git a/src/buildtool/execution_engine/executor/executor.hpp b/src/buildtool/execution_engine/executor/executor.hpp index 66790696..a25958b2 100644 --- a/src/buildtool/execution_engine/executor/executor.hpp +++ b/src/buildtool/execution_engine/executor/executor.hpp @@ -645,10 +645,13 @@ class ExecutorImpl { auto message = ""s; bool has_both = has_err and has_out; if (has_err or has_out) { - message += (has_both ? "Output"s - : has_out ? "Stdout"s - : "Stderr"s) + - " of command "; + if (has_both) { + message += "Output"s; + } + else { + message += has_out ? "Stdout"s : "Stderr"s; + } + message += " of command "; } message += nlohmann::json(action->Command()).dump() + " in environment " + @@ -694,14 +697,13 @@ class ExecutorImpl { logger.Emit(LogLevel::Error, "{}", msg.str()); } - [[nodiscard]] static inline auto ScaleTime(std::chrono::milliseconds t, - double f) - -> std::chrono::milliseconds { + [[nodiscard]] static auto ScaleTime(std::chrono::milliseconds t, + double f) -> std::chrono::milliseconds { return std::chrono::milliseconds( std::lround(static_cast<double>(t.count()) * f)); } - [[nodiscard]] static inline auto MergeProperties( + [[nodiscard]] static auto MergeProperties( const ExecutionProperties& base, const ExecutionProperties& overlay) { ExecutionProperties result = base; @@ -715,7 +717,7 @@ class ExecutorImpl { /// \brief Get the alternative endpoint based on a specified set of platform /// properties. These are checked against the dispatch list of an existing /// remote context. - [[nodiscard]] static inline auto GetAlternativeEndpoint( + [[nodiscard]] static auto GetAlternativeEndpoint( const ExecutionProperties& properties, const gsl::not_null<RemoteContext const*>& remote_context, const gsl::not_null<HashFunction const*>& hash_function) diff --git a/src/buildtool/file_system/file_root.hpp b/src/buildtool/file_system/file_root.hpp index 82e30b89..fed32177 100644 --- a/src/buildtool/file_system/file_root.hpp +++ b/src/buildtool/file_system/file_root.hpp @@ -667,9 +667,11 @@ class FileRoot { return std::pair(FileRoot{path}, std::move(path)); } if (root[0] == FileRoot::kGitTreeMarker) { - if (not(root.size() == 3 and root[1].is_string() and - root[2].is_string()) and - not(root.size() == 2 and root[1].is_string())) { + bool const has_one_arg = root.size() == 2 and root[1].is_string(); + bool const has_two_args = root.size() == 3 and + root[1].is_string() and + root[2].is_string(); + if (not has_one_arg and not has_two_args) { *error_msg = fmt::format( "\"git tree\" scheme expects one or two string " "arguments, but found {} for {} of repository {}", @@ -709,9 +711,11 @@ class FileRoot { std::move(path)); } if (root[0] == FileRoot::kGitTreeIgnoreSpecialMarker) { - if (not(root.size() == 3 and root[1].is_string() and - root[2].is_string()) and - not(root.size() == 2 and root[1].is_string())) { + bool const has_one_arg = root.size() == 2 and root[1].is_string(); + bool const has_two_args = root.size() == 3 and + root[1].is_string() and + root[2].is_string(); + if (not has_one_arg and not has_two_args) { *error_msg = fmt::format( "\"git tree ignore-special\" scheme expects one or two " "string arguments, but found {} for {} of repository {}", diff --git a/src/buildtool/file_system/file_system_manager.hpp b/src/buildtool/file_system/file_system_manager.hpp index 6e65fb59..ad3e4773 100644 --- a/src/buildtool/file_system/file_system_manager.hpp +++ b/src/buildtool/file_system/file_system_manager.hpp @@ -52,7 +52,7 @@ #include "src/utils/cpp/path.hpp" namespace detail { -static inline consteval auto BitWidth(int max_val) -> int { +static consteval auto BitWidth(int max_val) -> int { constexpr int kBitsPerByte = 8; int i = sizeof(max_val) * kBitsPerByte; while ((i-- > 0) and (((max_val >> i) & 0x01) == 0x00)) { // NOLINT diff --git a/src/buildtool/main/main.cpp b/src/buildtool/main/main.cpp index a972b80b..34ee14b1 100644 --- a/src/buildtool/main/main.cpp +++ b/src/buildtool/main/main.cpp @@ -61,7 +61,6 @@ #include "src/buildtool/serve_api/remote/serve_api.hpp" #include "src/buildtool/storage/config.hpp" #include "src/buildtool/storage/file_chunker.hpp" -#include "src/buildtool/storage/garbage_collector.hpp" #include "src/buildtool/storage/storage.hpp" #include "src/buildtool/storage/target_cache.hpp" #include "src/utils/cpp/concepts.hpp" diff --git a/src/buildtool/multithreading/async_map_utils.hpp b/src/buildtool/multithreading/async_map_utils.hpp index 47e5b5b4..2b2defeb 100644 --- a/src/buildtool/multithreading/async_map_utils.hpp +++ b/src/buildtool/multithreading/async_map_utils.hpp @@ -46,9 +46,13 @@ template <typename K, typename V> oss << fmt::format("Cycle detected in {}:", name) << std::endl; for (auto const& k : *cycle) { auto match = (k == cycle->back()); - auto prefix{match ? found ? "`-- "s : ".-> "s - : found ? "| "s - : " "s}; + std::string prefix; + if (match) { + prefix = found ? "`-- "s : ".-> "s; + } + else { + prefix = found ? "| "s : " "s; + } oss << prefix << key_printer(k) << std::endl; found = found or match; } diff --git a/src/buildtool/storage/file_chunker.cpp b/src/buildtool/storage/file_chunker.cpp index 08f0dc11..2a25cbf8 100644 --- a/src/buildtool/storage/file_chunker.cpp +++ b/src/buildtool/storage/file_chunker.cpp @@ -59,7 +59,7 @@ auto FileChunker::NextChunk() noexcept -> std::optional<std::string> { auto remaining = size_ - pos_; if (remaining < max_chunk_size_ and not stream_.eof()) { // Move the remaining bytes of the buffer to the front. - buffer_.copy(&buffer_[0], remaining, pos_); + buffer_.copy(buffer_.data(), remaining, pos_); auto ssize = static_cast<std::streamsize>(buffer_.size() - remaining); // Fill the buffer with stream content. stream_.read(&buffer_[remaining], ssize); diff --git a/src/other_tools/just_mr/setup.cpp b/src/other_tools/just_mr/setup.cpp index ba22936d..0c884d86 100644 --- a/src/other_tools/just_mr/setup.cpp +++ b/src/other_tools/just_mr/setup.cpp @@ -366,7 +366,7 @@ auto MultiRepoSetup(std::shared_ptr<Configuration> const& config, nlohmann::json mr_repos{}; for (auto const& repo : setup_repos->to_setup) { auto i = static_cast<std::size_t>( - &repo - &setup_repos->to_setup[0]); // get index + &repo - setup_repos->to_setup.data()); // get index mr_repos[repo] = *values[i]; } // populate ALT_DIRS diff --git a/src/other_tools/just_mr/update.cpp b/src/other_tools/just_mr/update.cpp index 1c1626eb..587a0f8f 100644 --- a/src/other_tools/just_mr/update.cpp +++ b/src/other_tools/just_mr/update.cpp @@ -257,7 +257,7 @@ auto MultiRepoUpdate(std::shared_ptr<Configuration> const& config, for (auto const& repo_name : repos_to_update_names) { auto i = static_cast<std::size_t>( &repo_name - - &repos_to_update_names[0]); // get index + repos_to_update_names.data()); // get index // we know "repository" is a map for repo_name, so // field "commit" is here either overwritten or set if // missing; either way, this should always work diff --git a/src/utils/cpp/tmp_dir.cpp b/src/utils/cpp/tmp_dir.cpp index 11f10559..db41512b 100644 --- a/src/utils/cpp/tmp_dir.cpp +++ b/src/utils/cpp/tmp_dir.cpp @@ -40,7 +40,7 @@ auto TmpDir::Create(std::filesystem::path const& prefix, // attempt to make the tmp dir // NOLINTNEXTLINE(cppcoreguidelines-pro-type-vararg,hicpp-vararg) - char* tmp_dir = mkdtemp(&c_tmpl[0]); + char* tmp_dir = mkdtemp(c_tmpl.data()); if (tmp_dir == nullptr) { return nullptr; } diff --git a/test/buildtool/execution_api/common/bytestream_utils.test.cpp b/test/buildtool/execution_api/common/bytestream_utils.test.cpp index 1468b156..a2f139e7 100644 --- a/test/buildtool/execution_api/common/bytestream_utils.test.cpp +++ b/test/buildtool/execution_api/common/bytestream_utils.test.cpp @@ -35,7 +35,7 @@ TEST_CASE("ReadRequest", "[common]") { ByteStreamUtils::ReadRequest{kInstanceName, digest}.ToString(); auto const parsed = ByteStreamUtils::ReadRequest::FromString(request); REQUIRE(parsed); - CHECK(parsed->GetInstanceName().compare(kInstanceName) == 0); + CHECK(parsed->GetInstanceName() == kInstanceName); CHECK(std::equal_to<bazel_re::Digest>{}(parsed->GetDigest(), digest)); } @@ -54,7 +54,7 @@ TEST_CASE("WriteRequest", "[common]") { ByteStreamUtils::WriteRequest{kInstanceName, uuid, digest}.ToString(); auto const parsed = ByteStreamUtils::WriteRequest::FromString(request); REQUIRE(parsed); - CHECK(parsed->GetInstanceName().compare(kInstanceName) == 0); - CHECK(parsed->GetUUID().compare(uuid) == 0); + CHECK(parsed->GetInstanceName() == kInstanceName); + CHECK(parsed->GetUUID() == uuid); CHECK(std::equal_to<bazel_re::Digest>{}(parsed->GetDigest(), digest)); } diff --git a/test/utils/large_objects/large_object_utils.cpp b/test/utils/large_objects/large_object_utils.cpp index a30a9276..a9342b23 100644 --- a/test/utils/large_objects/large_object_utils.cpp +++ b/test/utils/large_objects/large_object_utils.cpp @@ -30,7 +30,7 @@ class Randomizer final { Randomizer(std::uint64_t min, std::uint64_t max) noexcept : range_(std::random_device{}()), distribution_(min, max) {} - [[nodiscard]] inline auto Get() noexcept -> std::uint64_t { + [[nodiscard]] auto Get() noexcept -> std::uint64_t { return distribution_(range_); } |