From 1fa6a3e3354bd98707303476b5a54147ccdd533a Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 29 Mar 2024 20:26:38 -0700 Subject: [PATCH 1/2] 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"; From 194a1b91af6d8848e4cc0dfbdcc153ee2dbed140 Mon Sep 17 00:00:00 2001 From: Jade Lovelace Date: Fri, 29 Mar 2024 20:26:38 -0700 Subject: [PATCH 2/2] Make things that can throw not noexcept anymore This does involve making a large number of destructors able to throw, because we had to change it high in the class hierarchy. Oh well. Change-Id: Ib62d3d6895b755f20322bb8acc9bf43daf0174b2 --- src/libstore/build/derivation-goal.cc | 2 +- src/libstore/build/derivation-goal.hh | 2 +- src/libstore/build/goal.hh | 2 +- src/libstore/build/local-derivation-goal.cc | 2 +- src/libstore/build/local-derivation-goal.hh | 2 +- src/libstore/store-api.hh | 4 ++-- src/libutil/util.cc | 2 +- src/libutil/util.hh | 4 ++-- 8 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/libstore/build/derivation-goal.cc b/src/libstore/build/derivation-goal.cc index 5e0795115..f879a580e 100644 --- a/src/libstore/build/derivation-goal.cc +++ b/src/libstore/build/derivation-goal.cc @@ -108,7 +108,7 @@ DerivationGoal::DerivationGoal(const StorePath & drvPath, const BasicDerivation } -DerivationGoal::~DerivationGoal() +DerivationGoal::~DerivationGoal() noexcept(false) { /* Careful: we should never ever throw an exception from a destructor. */ diff --git a/src/libstore/build/derivation-goal.hh b/src/libstore/build/derivation-goal.hh index ddb5ee1e3..28aa6afbe 100644 --- a/src/libstore/build/derivation-goal.hh +++ b/src/libstore/build/derivation-goal.hh @@ -215,7 +215,7 @@ struct DerivationGoal : public Goal DerivationGoal(const StorePath & drvPath, const BasicDerivation & drv, const OutputsSpec & wantedOutputs, Worker & worker, BuildMode buildMode = bmNormal); - virtual ~DerivationGoal(); + virtual ~DerivationGoal() noexcept(false); void timedOut(Error && ex) override; diff --git a/src/libstore/build/goal.hh b/src/libstore/build/goal.hh index 9af083230..5b755c275 100644 --- a/src/libstore/build/goal.hh +++ b/src/libstore/build/goal.hh @@ -127,7 +127,7 @@ public: : worker(worker) { } - virtual ~Goal() + virtual ~Goal() noexcept(false) { trace("goal destroyed"); } diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc index ef170d815..0709dcbcb 100644 --- a/src/libstore/build/local-derivation-goal.cc +++ b/src/libstore/build/local-derivation-goal.cc @@ -96,7 +96,7 @@ void handleDiffHook( const Path LocalDerivationGoal::homeDir = "/homeless-shelter"; -LocalDerivationGoal::~LocalDerivationGoal() +LocalDerivationGoal::~LocalDerivationGoal() noexcept(false) { /* Careful: we should never ever throw an exception from a destructor. */ diff --git a/src/libstore/build/local-derivation-goal.hh b/src/libstore/build/local-derivation-goal.hh index 05c2c3a56..b7f317fb6 100644 --- a/src/libstore/build/local-derivation-goal.hh +++ b/src/libstore/build/local-derivation-goal.hh @@ -179,7 +179,7 @@ struct LocalDerivationGoal : public DerivationGoal using DerivationGoal::DerivationGoal; - virtual ~LocalDerivationGoal() override; + virtual ~LocalDerivationGoal() noexcept(false) override; /** * Whether we need to perform hash rewriting if there are valid output paths. diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh index 88c182b94..cb9f8e4a6 100644 --- a/src/libstore/store-api.hh +++ b/src/libstore/store-api.hh @@ -108,7 +108,7 @@ struct StoreConfig : public Config static StringSet getDefaultSystemFeatures(); - virtual ~StoreConfig() { } + virtual ~StoreConfig() noexcept(false) { } /** * The name of this type of store. @@ -220,7 +220,7 @@ public: */ virtual void init() {}; - virtual ~Store() { } + virtual ~Store() noexcept(false) { } virtual std::string getUri() = 0; diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 9b65bd77f..5cd4df8e6 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -947,7 +947,7 @@ Pid::Pid(pid_t pid) } -Pid::~Pid() +Pid::~Pid() noexcept(false) { if (pid != -1) kill(); } diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 860ddae06..29a70447e 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -335,7 +335,7 @@ public: AutoCloseFD(AutoCloseFD&& fd); ~AutoCloseFD(); AutoCloseFD& operator =(const AutoCloseFD & fd) = delete; - AutoCloseFD& operator =(AutoCloseFD&& fd); + AutoCloseFD& operator =(AutoCloseFD&& fd) noexcept(false); int get() const; explicit operator bool() const; int release(); @@ -384,7 +384,7 @@ class Pid public: Pid(); Pid(pid_t pid); - ~Pid(); + ~Pid() noexcept(false); void operator =(pid_t pid); operator pid_t(); int kill();