From 1fa6a3e3354bd98707303476b5a54147ccdd533a Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 29 Mar 2024 20:26:38 -0700 Subject: [PATCH] Fix various clang-tidy lints * some things that can throw are marked noexcept yet the linter seems to think not. Maybe they can't throw in practice. I would rather not have the UB possibility in pretty obvious cold paths. * various default-case-missing complaints * a fair pile of casts from integer to character, which are in fact deliberate. * an instance of * bugprone-not-null-terminated-result on handing a string to curl in chunks of bytes. our usage is fine. * reassigning a unique_ptr by CRIMES instead of using release(), then using release() and ignoring the result. wild. let's use release() for its intended purpose. Change-Id: Ic3e7affef12383576213a8a7c8145c27e662513d --- src/libstore/content-address.cc | 4 +-- src/libstore/content-address.hh | 4 +-- src/libstore/filetransfer.cc | 5 +++- src/libstore/local-store.cc | 8 +++-- src/libutil/logging.cc | 4 +-- src/libutil/source-path.cc | 1 + tests/unit/libstore-support/tests/path.cc | 6 ++-- tests/unit/libstore/outputs-spec.cc | 30 ++++++++++--------- .../tests/terminal-code-eater.cc | 1 + 9 files changed, 36 insertions(+), 27 deletions(-) diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc index 77f23b0b3..811ddbed7 100644 --- a/src/libstore/content-address.cc +++ b/src/libstore/content-address.cc @@ -157,7 +157,7 @@ size_t StoreReferences::size() const return (self ? 1 : 0) + others.size(); } -ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const ContentAddress & ca) noexcept +ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const ContentAddress & ca) { return std::visit(overloaded { [&](const TextIngestionMethod &) -> ContentAddressWithReferences { @@ -177,7 +177,7 @@ ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const Con } std::optional ContentAddressWithReferences::fromPartsOpt( - ContentAddressMethod method, Hash hash, StoreReferences refs) noexcept + ContentAddressMethod method, Hash hash, StoreReferences refs) { return std::visit(overloaded { [&](TextIngestionMethod _) -> std::optional { diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh index c4d619bdc..200543704 100644 --- a/src/libstore/content-address.hh +++ b/src/libstore/content-address.hh @@ -255,7 +255,7 @@ struct ContentAddressWithReferences * Create a `ContentAddressWithReferences` from a mere * `ContentAddress`, by claiming no references. */ - static ContentAddressWithReferences withoutRefs(const ContentAddress &) noexcept; + static ContentAddressWithReferences withoutRefs(const ContentAddress &); /** * Create a `ContentAddressWithReferences` from 3 parts: @@ -270,7 +270,7 @@ struct ContentAddressWithReferences * returns for invalid combinations. */ static std::optional fromPartsOpt( - ContentAddressMethod method, Hash hash, StoreReferences refs) noexcept; + ContentAddressMethod method, Hash hash, StoreReferences refs); ContentAddressMethod getMethod() const; diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index b843a95f9..ef2480863 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -142,7 +142,7 @@ struct curlFileTransfer : public FileTransfer template void fail(T && e) { - failEx(std::make_exception_ptr(std::move(e))); + failEx(std::make_exception_ptr(std::forward(e))); } LambdaSink finalSink; @@ -270,6 +270,9 @@ struct curlFileTransfer : public FileTransfer return 0; auto count = std::min(size * nitems, request.data->length() - readOffset); assert(count); + // Lint: this is turning a string into a byte array to hand to + // curl, which is fine. + // NOLINTNEXTLINE(bugprone-not-null-terminated-result) memcpy(buffer, request.data->data() + readOffset, count); readOffset += count; return count; diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc index db3934d5e..f252b449c 100644 --- a/src/libstore/local-store.cc +++ b/src/libstore/local-store.cc @@ -1315,10 +1315,12 @@ StorePath LocalStore::addToStoreFromDump(Source & source0, std::string_view name auto oldSize = dump.size(); constexpr size_t chunkSize = 65536; auto want = std::min(chunkSize, settings.narBufferSize - oldSize); - if (auto tmp = realloc(dumpBuffer.get(), oldSize + want)) { - dumpBuffer.release(); - dumpBuffer.reset((char*) tmp); + + auto *toRealloc = dumpBuffer.release(); + if (auto realloced = realloc(toRealloc, oldSize + want)) { + dumpBuffer.reset((char*) realloced); } else { + free(toRealloc); throw std::bad_alloc(); } auto got = 0; diff --git a/src/libutil/logging.cc b/src/libutil/logging.cc index 691b77e95..0e63f9bd6 100644 --- a/src/libutil/logging.cc +++ b/src/libutil/logging.cc @@ -69,8 +69,8 @@ public: case lvlWarn: c = '4'; break; case lvlNotice: case lvlInfo: c = '5'; break; case lvlTalkative: case lvlChatty: c = '6'; break; - case lvlDebug: case lvlVomit: c = '7'; break; - default: c = '7'; break; // should not happen, and missing enum case is reported by -Werror=switch-enum + case lvlDebug: case lvlVomit: + default: c = '7'; break; // default case should not happen, and missing enum case is reported by -Werror=switch-enum } prefix = std::string("<") + c + ">"; } diff --git a/src/libutil/source-path.cc b/src/libutil/source-path.cc index e6721f808..cfaac20c0 100644 --- a/src/libutil/source-path.cc +++ b/src/libutil/source-path.cc @@ -50,6 +50,7 @@ InputAccessor::DirEntries SourcePath::readDirectory() const case DT_REG: type = InputAccessor::Type::tRegular; break; case DT_LNK: type = InputAccessor::Type::tSymlink; break; case DT_DIR: type = InputAccessor::Type::tDirectory; break; + default: break; // unknown type } res.emplace(entry.name, type); } diff --git a/tests/unit/libstore-support/tests/path.cc b/tests/unit/libstore-support/tests/path.cc index 8bf501ab6..ffc4fc607 100644 --- a/tests/unit/libstore-support/tests/path.cc +++ b/tests/unit/libstore-support/tests/path.cc @@ -32,13 +32,13 @@ Gen Arbitrary::arbitrary() for (size_t c = 0; c < len; ++c) { switch (auto i = *gen::inRange(0, 10 + 2 * 26 + 6)) { case 0 ... 9: - pre += '0' + i; + pre += static_cast('0' + i); break; case 10 ... 35: - pre += 'A' + (i - 10); + pre += static_cast('A' + (i - 10)); break; case 36 ... 61: - pre += 'a' + (i - 36); + pre += static_cast('a' + (i - 36)); break; case 62: pre += '+'; diff --git a/tests/unit/libstore/outputs-spec.cc b/tests/unit/libstore/outputs-spec.cc index 63cde681b..72a7da974 100644 --- a/tests/unit/libstore/outputs-spec.cc +++ b/tests/unit/libstore/outputs-spec.cc @@ -170,20 +170,22 @@ TEST(ExtendedOutputsSpec, many_carrot) { } -#define TEST_JSON(TYPE, NAME, STR, VAL) \ - \ - TEST(TYPE, NAME ## _to_json) { \ - using nlohmann::literals::operator "" _json; \ - ASSERT_EQ( \ - STR ## _json, \ - ((nlohmann::json) TYPE { VAL })); \ - } \ - \ - TEST(TYPE, NAME ## _from_json) { \ - using nlohmann::literals::operator "" _json; \ - ASSERT_EQ( \ - TYPE { VAL }, \ - (STR ## _json).get()); \ +#define TEST_JSON(TYPE, NAME, STR, VAL) \ + \ + TEST(TYPE, NAME ## _to_json) { \ + using nlohmann::literals::operator "" _json; \ + ASSERT_EQ( \ + STR ## _json, \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ + ((nlohmann::json) TYPE { VAL })); \ + } \ + \ + TEST(TYPE, NAME ## _from_json) { \ + using nlohmann::literals::operator "" _json; \ + ASSERT_EQ( \ + /* NOLINTNEXTLINE(bugprone-macro-parentheses) */ \ + TYPE { VAL }, \ + (STR ## _json).get()); \ } TEST_JSON(OutputsSpec, all, R"(["*"])", OutputsSpec::All { }) diff --git a/tests/unit/libutil-support/tests/terminal-code-eater.cc b/tests/unit/libutil-support/tests/terminal-code-eater.cc index ad05ed1f6..6e108009a 100644 --- a/tests/unit/libutil-support/tests/terminal-code-eater.cc +++ b/tests/unit/libutil-support/tests/terminal-code-eater.cc @@ -26,6 +26,7 @@ void TerminalCodeEater::feed(char c, std::function on_char) // Just eat \r, since it is part of clearing a line case '\r': return; + default: break; } if constexpr (DEBUG_EATER) { std::cerr << "eater uneat" << MaybeHexEscapedChar{c} << "\n";