From d3fcedbcf5afaadfbe1d239ef0289cd12b56ee9d Mon Sep 17 00:00:00 2001 From: Pierre Bourdon Date: Tue, 27 Aug 2024 00:43:17 +0200 Subject: [PATCH] treewide: enable clang-tidy bugprone findings Fix some trivial findings throughout the codebase, mostly making implicit casts explicit. --- .clang-tidy | 8 +++++ src/hydra-evaluator/hydra-evaluator.cc | 9 ++++-- src/hydra-queue-runner/build-remote.cc | 3 +- src/hydra-queue-runner/builder.cc | 4 +-- src/hydra-queue-runner/dispatcher.cc | 8 ++--- src/hydra-queue-runner/hydra-queue-runner.cc | 32 ++++++++++---------- src/hydra-queue-runner/queue-monitor.cc | 6 ++-- src/hydra-queue-runner/state.hh | 6 ++-- 8 files changed, 45 insertions(+), 31 deletions(-) create mode 100644 .clang-tidy diff --git a/.clang-tidy b/.clang-tidy new file mode 100644 index 00000000..b8567b2e --- /dev/null +++ b/.clang-tidy @@ -0,0 +1,8 @@ +UseColor: true +Checks: + - -* + - bugprone-* + # kind of nonsense + - -bugprone-easily-swappable-parameters + # many warnings due to not recognizing `assert` properly + - -bugprone-unchecked-optional-access diff --git a/src/hydra-evaluator/hydra-evaluator.cc b/src/hydra-evaluator/hydra-evaluator.cc index 462c085e..59ffabed 100644 --- a/src/hydra-evaluator/hydra-evaluator.cc +++ b/src/hydra-evaluator/hydra-evaluator.cc @@ -88,7 +88,7 @@ struct Evaluator JobsetId name; std::optional evaluation_style; time_t lastCheckedTime, triggerTime; - int checkInterval; + time_t checkInterval; Pid pid; }; @@ -144,7 +144,9 @@ struct Evaluator jobset.lastCheckedTime = row["lastCheckedTime"].as(0); jobset.triggerTime = row["triggerTime"].as(notTriggered); jobset.checkInterval = row["checkInterval"].as(); - switch (row["jobset_enabled"].as(0)) { + + int eval_style = row["jobset_enabled"].as(0); + switch (eval_style) { case 1: jobset.evaluation_style = EvaluationStyle::SCHEDULE; break; @@ -154,6 +156,9 @@ struct Evaluator case 3: jobset.evaluation_style = EvaluationStyle::ONE_AT_A_TIME; break; + default: + // Disabled or unknown. Leave as nullopt. + break; } seen.insert(name); diff --git a/src/hydra-queue-runner/build-remote.cc b/src/hydra-queue-runner/build-remote.cc index fcab7b54..cde1e84b 100644 --- a/src/hydra-queue-runner/build-remote.cc +++ b/src/hydra-queue-runner/build-remote.cc @@ -41,6 +41,7 @@ static Strings extraStoreArgs(std::string & machine) } } catch (BadURL &) { // We just try to continue with `machine->sshName` here for backwards compat. + printMsg(lvlWarn, "could not parse machine URL '%s', passing through to SSH", machine); } return result; @@ -697,7 +698,7 @@ void State::buildRemote(ref destStore, if (info->consecutiveFailures == 0 || info->lastFailure < now - std::chrono::seconds(30)) { info->consecutiveFailures = std::min(info->consecutiveFailures + 1, (unsigned int) 4); info->lastFailure = now; - int delta = retryInterval * std::pow(retryBackoff, info->consecutiveFailures - 1) + (rand() % 30); + int delta = static_cast(retryInterval * std::pow(retryBackoff, info->consecutiveFailures - 1) + (rand() % 30)); printMsg(lvlInfo, "will disable machine ‘%1%’ for %2%s", machine->sshName, delta); info->disabledUntil = now + std::chrono::seconds(delta); } diff --git a/src/hydra-queue-runner/builder.cc b/src/hydra-queue-runner/builder.cc index 642d6ba5..a5109f62 100644 --- a/src/hydra-queue-runner/builder.cc +++ b/src/hydra-queue-runner/builder.cc @@ -72,7 +72,7 @@ void State::builder(MachineReservation::ptr reservation) step_->tries++; nrRetries++; if (step_->tries > maxNrRetries) maxNrRetries = step_->tries; // yeah yeah, not atomic - int delta = retryInterval * std::pow(retryBackoff, step_->tries - 1) + (rand() % 10); + int delta = static_cast(retryInterval * std::pow(retryBackoff, step_->tries - 1) + (rand() % 10)); printMsg(lvlInfo, "will retry ‘%s’ after %ss", localStore->printStorePath(step->drvPath), delta); step_->after = std::chrono::system_clock::now() + std::chrono::seconds(delta); } @@ -251,7 +251,7 @@ State::StepResult State::doBuildStep(nix::ref destStore, auto step_(step->state.lock()); if (!step_->jobsets.empty()) { // FIXME: loss of precision. - time_t charge = (result.stopTime - result.startTime) / step_->jobsets.size(); + time_t charge = (result.stopTime - result.startTime) / static_cast(step_->jobsets.size()); for (auto & jobset : step_->jobsets) jobset->addStep(result.startTime, charge); } diff --git a/src/hydra-queue-runner/dispatcher.cc b/src/hydra-queue-runner/dispatcher.cc index 03b42433..a477321e 100644 --- a/src/hydra-queue-runner/dispatcher.cc +++ b/src/hydra-queue-runner/dispatcher.cc @@ -46,7 +46,7 @@ void State::dispatcher() auto t_after_work = std::chrono::steady_clock::now(); prom.dispatcher_time_spent_running.Increment( - std::chrono::duration_cast(t_after_work - t_before_work).count()); + static_cast(std::chrono::duration_cast(t_after_work - t_before_work).count())); dispatchTimeMs += std::chrono::duration_cast(t_after_work - t_before_work).count(); /* Sleep until we're woken up (either because a runnable build @@ -63,7 +63,7 @@ void State::dispatcher() auto t_after_sleep = std::chrono::steady_clock::now(); prom.dispatcher_time_spent_waiting.Increment( - std::chrono::duration_cast(t_after_sleep - t_after_work).count()); + static_cast(std::chrono::duration_cast(t_after_sleep - t_after_work).count())); } catch (std::exception & e) { printError("dispatcher: %s", e.what()); @@ -243,8 +243,8 @@ system_time State::doDispatch() sort(machinesSorted.begin(), machinesSorted.end(), [](const MachineInfo & a, const MachineInfo & b) -> bool { - float ta = std::round(a.currentJobs / a.machine->speedFactorFloat); - float tb = std::round(b.currentJobs / b.machine->speedFactorFloat); + float ta = std::round(static_cast(a.currentJobs) / a.machine->speedFactorFloat); + float tb = std::round(static_cast(b.currentJobs) / b.machine->speedFactorFloat); return ta != tb ? ta < tb : a.machine->speedFactorFloat != b.machine->speedFactorFloat ? a.machine->speedFactorFloat > b.machine->speedFactorFloat : diff --git a/src/hydra-queue-runner/hydra-queue-runner.cc b/src/hydra-queue-runner/hydra-queue-runner.cc index b5f0032f..71acfdbb 100644 --- a/src/hydra-queue-runner/hydra-queue-runner.cc +++ b/src/hydra-queue-runner/hydra-queue-runner.cc @@ -105,7 +105,7 @@ State::State(std::optional metricsAddrOpt) : config(std::make_unique()) , maxUnsupportedTime(config->getIntOption("max_unsupported_time", 0)) , dbPool(config->getIntOption("max_db_connections", 128)) - , localWorkThrottler(config->getIntOption("max_local_worker_threads", std::min(maxSupportedLocalWorkers, std::max(4u, std::thread::hardware_concurrency()) - 2))) + , localWorkThrottler(static_cast(config->getIntOption("max_local_worker_threads", std::min(maxSupportedLocalWorkers, std::max(4u, std::thread::hardware_concurrency()) - 2)))) , maxOutputSize(config->getIntOption("max_output_size", 2ULL << 30)) , maxLogSize(config->getIntOption("max_log_size", 64ULL << 20)) , uploadLogsToBinaryCache(config->getBoolOption("upload_logs_to_binary_cache", false)) @@ -198,7 +198,7 @@ void State::parseMachines(const std::string & contents) machine->sshName = tokens[0]; machine->systemTypesSet = tokenizeString(tokens[1], ","); - machine->speedFactorFloat = atof(tokens[4].c_str()); + machine->speedFactorFloat = static_cast(atof(tokens[4].c_str())); /* Re-use the State object of the previous machine with the same name. */ @@ -412,7 +412,7 @@ void State::finishBuildStep(pqxx::work & txn, const RemoteResult & result, } -int State::createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t stopTime, +unsigned int State::createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t stopTime, Build::ptr build, const StorePath & drvPath, const nix::Derivation drv, const std::string & outputName, const StorePath & storePath) { restart: @@ -620,7 +620,7 @@ void State::dumpStatus(Connection & conn) {"bytesReceived", bytesReceived.load()}, {"nrBuildsRead", nrBuildsRead.load()}, {"buildReadTimeMs", buildReadTimeMs.load()}, - {"buildReadTimeAvgMs", nrBuildsRead == 0 ? 0.0 : (float) buildReadTimeMs / nrBuildsRead}, + {"buildReadTimeAvgMs", nrBuildsRead == 0 ? 0.0 : (float) buildReadTimeMs / (float) nrBuildsRead}, {"nrBuildsDone", nrBuildsDone.load()}, {"nrStepsStarted", nrStepsStarted.load()}, {"nrStepsDone", nrStepsDone.load()}, @@ -629,7 +629,7 @@ void State::dumpStatus(Connection & conn) {"nrQueueWakeups", nrQueueWakeups.load()}, {"nrDispatcherWakeups", nrDispatcherWakeups.load()}, {"dispatchTimeMs", dispatchTimeMs.load()}, - {"dispatchTimeAvgMs", nrDispatcherWakeups == 0 ? 0.0 : (float) dispatchTimeMs / nrDispatcherWakeups}, + {"dispatchTimeAvgMs", nrDispatcherWakeups == 0 ? 0.0 : (float) dispatchTimeMs / (float) nrDispatcherWakeups}, {"nrDbConnections", dbPool.count()}, {"nrActiveDbUpdates", nrActiveDbUpdates.load()}, }; @@ -649,8 +649,8 @@ void State::dumpStatus(Connection & conn) if (nrStepsDone) { statusJson["totalStepTime"] = totalStepTime.load(); statusJson["totalStepBuildTime"] = totalStepBuildTime.load(); - statusJson["avgStepTime"] = (float) totalStepTime / nrStepsDone; - statusJson["avgStepBuildTime"] = (float) totalStepBuildTime / nrStepsDone; + statusJson["avgStepTime"] = (float) totalStepTime / (float) nrStepsDone; + statusJson["avgStepBuildTime"] = (float) totalStepBuildTime / (float) nrStepsDone; } { @@ -677,8 +677,8 @@ void State::dumpStatus(Connection & conn) if (m->state->nrStepsDone) { machine["totalStepTime"] = s->totalStepTime.load(); machine["totalStepBuildTime"] = s->totalStepBuildTime.load(); - machine["avgStepTime"] = (float) s->totalStepTime / s->nrStepsDone; - machine["avgStepBuildTime"] = (float) s->totalStepBuildTime / s->nrStepsDone; + machine["avgStepTime"] = (float) s->totalStepTime / (float) s->nrStepsDone; + machine["avgStepBuildTime"] = (float) s->totalStepBuildTime / (float) s->nrStepsDone; } statusJson["machines"][m->sshName] = machine; } @@ -732,11 +732,11 @@ void State::dumpStatus(Connection & conn) {"narWriteCompressionTimeMs", stats.narWriteCompressionTimeMs.load()}, {"narCompressionSavings", stats.narWriteBytes - ? 1.0 - (double) stats.narWriteCompressedBytes / stats.narWriteBytes + ? 1.0 - (double) stats.narWriteCompressedBytes / (double) stats.narWriteBytes : 0.0}, {"narCompressionSpeed", // MiB/s stats.narWriteCompressionTimeMs - ? (double) stats.narWriteBytes / stats.narWriteCompressionTimeMs * 1000.0 / (1024.0 * 1024.0) + ? (double) stats.narWriteBytes / (double) stats.narWriteCompressionTimeMs * 1000.0 / (1024.0 * 1024.0) : 0.0}, }; @@ -749,20 +749,20 @@ void State::dumpStatus(Connection & conn) {"putTimeMs", s3Stats.putTimeMs.load()}, {"putSpeed", s3Stats.putTimeMs - ? (double) s3Stats.putBytes / s3Stats.putTimeMs * 1000.0 / (1024.0 * 1024.0) + ? (double) s3Stats.putBytes / (double) s3Stats.putTimeMs * 1000.0 / (1024.0 * 1024.0) : 0.0}, {"get", s3Stats.get.load()}, {"getBytes", s3Stats.getBytes.load()}, {"getTimeMs", s3Stats.getTimeMs.load()}, {"getSpeed", s3Stats.getTimeMs - ? (double) s3Stats.getBytes / s3Stats.getTimeMs * 1000.0 / (1024.0 * 1024.0) + ? (double) s3Stats.getBytes / (double) s3Stats.getTimeMs * 1000.0 / (1024.0 * 1024.0) : 0.0}, {"head", s3Stats.head.load()}, {"costDollarApprox", - (s3Stats.get + s3Stats.head) / 10000.0 * 0.004 - + s3Stats.put / 1000.0 * 0.005 + - + s3Stats.getBytes / (1024.0 * 1024.0 * 1024.0) * 0.09}, + (double) (s3Stats.get + s3Stats.head) / 10000.0 * 0.004 + + (double) s3Stats.put / 1000.0 * 0.005 + + + (double) s3Stats.getBytes / (1024.0 * 1024.0 * 1024.0) * 0.09}, }; } } diff --git a/src/hydra-queue-runner/queue-monitor.cc b/src/hydra-queue-runner/queue-monitor.cc index 9cc4357e..019d4e6f 100644 --- a/src/hydra-queue-runner/queue-monitor.cc +++ b/src/hydra-queue-runner/queue-monitor.cc @@ -52,7 +52,7 @@ void State::queueMonitorLoop(Connection & conn) auto t_after_work = std::chrono::steady_clock::now(); prom.queue_monitor_time_spent_running.Increment( - std::chrono::duration_cast(t_after_work - t_before_work).count()); + static_cast(std::chrono::duration_cast(t_after_work - t_before_work).count())); /* Sleep until we get notification from the database about an event. */ @@ -79,7 +79,7 @@ void State::queueMonitorLoop(Connection & conn) auto t_after_sleep = std::chrono::steady_clock::now(); prom.queue_monitor_time_spent_waiting.Increment( - std::chrono::duration_cast(t_after_sleep - t_after_work).count()); + static_cast(std::chrono::duration_cast(t_after_sleep - t_after_work).count())); } exit(0); @@ -355,7 +355,7 @@ void State::processQueueChange(Connection & conn) pqxx::work txn(conn); auto res = txn.exec("select id, globalPriority from Builds where finished = 0"); for (auto const & row : res) - currentIds[row["id"].as()] = row["globalPriority"].as(); + currentIds[row["id"].as()] = row["globalPriority"].as(); } { diff --git a/src/hydra-queue-runner/state.hh b/src/hydra-queue-runner/state.hh index d0316b2a..bc3241d7 100644 --- a/src/hydra-queue-runner/state.hh +++ b/src/hydra-queue-runner/state.hh @@ -98,7 +98,7 @@ public: typedef std::shared_ptr ptr; typedef std::weak_ptr wptr; - static const time_t schedulingWindow = 24 * 60 * 60; + static const time_t schedulingWindow = static_cast(24 * 60 * 60); private: @@ -115,7 +115,7 @@ public: return (double) seconds / shares; } - void setShares(int shares_) + void setShares(unsigned int shares_) { assert(shares_ > 0); shares = shares_; @@ -534,7 +534,7 @@ private: void finishBuildStep(pqxx::work & txn, const RemoteResult & result, BuildID buildId, unsigned int stepNr, const std::string & machine); - int createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t stopTime, + unsigned int createSubstitutionStep(pqxx::work & txn, time_t startTime, time_t stopTime, Build::ptr build, const nix::StorePath & drvPath, const nix::Derivation drv, const std::string & outputName, const nix::StorePath & storePath); void updateBuild(pqxx::work & txn, Build::ptr build, BuildStatus status);