treewide: enable clang-tidy bugprone findings

Fix some trivial findings throughout the codebase, mostly making
implicit casts explicit.
This commit is contained in:
Pierre Bourdon 2024-08-27 00:43:17 +02:00
parent 3891ad77e3
commit d3fcedbcf5
Signed by: delroth
GPG key ID: 6FB80DCD84DA0F1C
8 changed files with 45 additions and 31 deletions

8
.clang-tidy Normal file
View file

@ -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

View file

@ -88,7 +88,7 @@ struct Evaluator
JobsetId name; JobsetId name;
std::optional<EvaluationStyle> evaluation_style; std::optional<EvaluationStyle> evaluation_style;
time_t lastCheckedTime, triggerTime; time_t lastCheckedTime, triggerTime;
int checkInterval; time_t checkInterval;
Pid pid; Pid pid;
}; };
@ -144,7 +144,9 @@ struct Evaluator
jobset.lastCheckedTime = row["lastCheckedTime"].as<time_t>(0); jobset.lastCheckedTime = row["lastCheckedTime"].as<time_t>(0);
jobset.triggerTime = row["triggerTime"].as<time_t>(notTriggered); jobset.triggerTime = row["triggerTime"].as<time_t>(notTriggered);
jobset.checkInterval = row["checkInterval"].as<time_t>(); jobset.checkInterval = row["checkInterval"].as<time_t>();
switch (row["jobset_enabled"].as<int>(0)) {
int eval_style = row["jobset_enabled"].as<int>(0);
switch (eval_style) {
case 1: case 1:
jobset.evaluation_style = EvaluationStyle::SCHEDULE; jobset.evaluation_style = EvaluationStyle::SCHEDULE;
break; break;
@ -154,6 +156,9 @@ struct Evaluator
case 3: case 3:
jobset.evaluation_style = EvaluationStyle::ONE_AT_A_TIME; jobset.evaluation_style = EvaluationStyle::ONE_AT_A_TIME;
break; break;
default:
// Disabled or unknown. Leave as nullopt.
break;
} }
seen.insert(name); seen.insert(name);

View file

@ -41,6 +41,7 @@ static Strings extraStoreArgs(std::string & machine)
} }
} catch (BadURL &) { } catch (BadURL &) {
// We just try to continue with `machine->sshName` here for backwards compat. // 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; return result;
@ -697,7 +698,7 @@ void State::buildRemote(ref<Store> destStore,
if (info->consecutiveFailures == 0 || info->lastFailure < now - std::chrono::seconds(30)) { if (info->consecutiveFailures == 0 || info->lastFailure < now - std::chrono::seconds(30)) {
info->consecutiveFailures = std::min(info->consecutiveFailures + 1, (unsigned int) 4); info->consecutiveFailures = std::min(info->consecutiveFailures + 1, (unsigned int) 4);
info->lastFailure = now; info->lastFailure = now;
int delta = retryInterval * std::pow(retryBackoff, info->consecutiveFailures - 1) + (rand() % 30); int delta = static_cast<int>(retryInterval * std::pow(retryBackoff, info->consecutiveFailures - 1) + (rand() % 30));
printMsg(lvlInfo, "will disable machine %1% for %2%s", machine->sshName, delta); printMsg(lvlInfo, "will disable machine %1% for %2%s", machine->sshName, delta);
info->disabledUntil = now + std::chrono::seconds(delta); info->disabledUntil = now + std::chrono::seconds(delta);
} }

View file

@ -72,7 +72,7 @@ void State::builder(MachineReservation::ptr reservation)
step_->tries++; step_->tries++;
nrRetries++; nrRetries++;
if (step_->tries > maxNrRetries) maxNrRetries = step_->tries; // yeah yeah, not atomic 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<int>(retryInterval * std::pow(retryBackoff, step_->tries - 1) + (rand() % 10));
printMsg(lvlInfo, "will retry %s after %ss", localStore->printStorePath(step->drvPath), delta); printMsg(lvlInfo, "will retry %s after %ss", localStore->printStorePath(step->drvPath), delta);
step_->after = std::chrono::system_clock::now() + std::chrono::seconds(delta); step_->after = std::chrono::system_clock::now() + std::chrono::seconds(delta);
} }
@ -251,7 +251,7 @@ State::StepResult State::doBuildStep(nix::ref<Store> destStore,
auto step_(step->state.lock()); auto step_(step->state.lock());
if (!step_->jobsets.empty()) { if (!step_->jobsets.empty()) {
// FIXME: loss of precision. // FIXME: loss of precision.
time_t charge = (result.stopTime - result.startTime) / step_->jobsets.size(); time_t charge = (result.stopTime - result.startTime) / static_cast<time_t>(step_->jobsets.size());
for (auto & jobset : step_->jobsets) for (auto & jobset : step_->jobsets)
jobset->addStep(result.startTime, charge); jobset->addStep(result.startTime, charge);
} }

View file

@ -46,7 +46,7 @@ void State::dispatcher()
auto t_after_work = std::chrono::steady_clock::now(); auto t_after_work = std::chrono::steady_clock::now();
prom.dispatcher_time_spent_running.Increment( prom.dispatcher_time_spent_running.Increment(
std::chrono::duration_cast<std::chrono::microseconds>(t_after_work - t_before_work).count()); static_cast<double>(std::chrono::duration_cast<std::chrono::microseconds>(t_after_work - t_before_work).count()));
dispatchTimeMs += std::chrono::duration_cast<std::chrono::milliseconds>(t_after_work - t_before_work).count(); dispatchTimeMs += std::chrono::duration_cast<std::chrono::milliseconds>(t_after_work - t_before_work).count();
/* Sleep until we're woken up (either because a runnable build /* 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(); auto t_after_sleep = std::chrono::steady_clock::now();
prom.dispatcher_time_spent_waiting.Increment( prom.dispatcher_time_spent_waiting.Increment(
std::chrono::duration_cast<std::chrono::microseconds>(t_after_sleep - t_after_work).count()); static_cast<double>(std::chrono::duration_cast<std::chrono::microseconds>(t_after_sleep - t_after_work).count()));
} catch (std::exception & e) { } catch (std::exception & e) {
printError("dispatcher: %s", e.what()); printError("dispatcher: %s", e.what());
@ -243,8 +243,8 @@ system_time State::doDispatch()
sort(machinesSorted.begin(), machinesSorted.end(), sort(machinesSorted.begin(), machinesSorted.end(),
[](const MachineInfo & a, const MachineInfo & b) -> bool [](const MachineInfo & a, const MachineInfo & b) -> bool
{ {
float ta = std::round(a.currentJobs / a.machine->speedFactorFloat); float ta = std::round(static_cast<float>(a.currentJobs) / a.machine->speedFactorFloat);
float tb = std::round(b.currentJobs / b.machine->speedFactorFloat); float tb = std::round(static_cast<float>(b.currentJobs) / b.machine->speedFactorFloat);
return return
ta != tb ? ta < tb : ta != tb ? ta < tb :
a.machine->speedFactorFloat != b.machine->speedFactorFloat ? a.machine->speedFactorFloat > b.machine->speedFactorFloat : a.machine->speedFactorFloat != b.machine->speedFactorFloat ? a.machine->speedFactorFloat > b.machine->speedFactorFloat :

View file

@ -105,7 +105,7 @@ State::State(std::optional<std::string> metricsAddrOpt)
: config(std::make_unique<HydraConfig>()) : config(std::make_unique<HydraConfig>())
, maxUnsupportedTime(config->getIntOption("max_unsupported_time", 0)) , maxUnsupportedTime(config->getIntOption("max_unsupported_time", 0))
, dbPool(config->getIntOption("max_db_connections", 128)) , 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<ptrdiff_t>(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)) , maxOutputSize(config->getIntOption("max_output_size", 2ULL << 30))
, maxLogSize(config->getIntOption("max_log_size", 64ULL << 20)) , maxLogSize(config->getIntOption("max_log_size", 64ULL << 20))
, uploadLogsToBinaryCache(config->getBoolOption("upload_logs_to_binary_cache", false)) , 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->sshName = tokens[0];
machine->systemTypesSet = tokenizeString<StringSet>(tokens[1], ","); machine->systemTypesSet = tokenizeString<StringSet>(tokens[1], ",");
machine->speedFactorFloat = atof(tokens[4].c_str()); machine->speedFactorFloat = static_cast<float>(atof(tokens[4].c_str()));
/* Re-use the State object of the previous machine with the /* Re-use the State object of the previous machine with the
same name. */ 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) Build::ptr build, const StorePath & drvPath, const nix::Derivation drv, const std::string & outputName, const StorePath & storePath)
{ {
restart: restart:
@ -620,7 +620,7 @@ void State::dumpStatus(Connection & conn)
{"bytesReceived", bytesReceived.load()}, {"bytesReceived", bytesReceived.load()},
{"nrBuildsRead", nrBuildsRead.load()}, {"nrBuildsRead", nrBuildsRead.load()},
{"buildReadTimeMs", buildReadTimeMs.load()}, {"buildReadTimeMs", buildReadTimeMs.load()},
{"buildReadTimeAvgMs", nrBuildsRead == 0 ? 0.0 : (float) buildReadTimeMs / nrBuildsRead}, {"buildReadTimeAvgMs", nrBuildsRead == 0 ? 0.0 : (float) buildReadTimeMs / (float) nrBuildsRead},
{"nrBuildsDone", nrBuildsDone.load()}, {"nrBuildsDone", nrBuildsDone.load()},
{"nrStepsStarted", nrStepsStarted.load()}, {"nrStepsStarted", nrStepsStarted.load()},
{"nrStepsDone", nrStepsDone.load()}, {"nrStepsDone", nrStepsDone.load()},
@ -629,7 +629,7 @@ void State::dumpStatus(Connection & conn)
{"nrQueueWakeups", nrQueueWakeups.load()}, {"nrQueueWakeups", nrQueueWakeups.load()},
{"nrDispatcherWakeups", nrDispatcherWakeups.load()}, {"nrDispatcherWakeups", nrDispatcherWakeups.load()},
{"dispatchTimeMs", dispatchTimeMs.load()}, {"dispatchTimeMs", dispatchTimeMs.load()},
{"dispatchTimeAvgMs", nrDispatcherWakeups == 0 ? 0.0 : (float) dispatchTimeMs / nrDispatcherWakeups}, {"dispatchTimeAvgMs", nrDispatcherWakeups == 0 ? 0.0 : (float) dispatchTimeMs / (float) nrDispatcherWakeups},
{"nrDbConnections", dbPool.count()}, {"nrDbConnections", dbPool.count()},
{"nrActiveDbUpdates", nrActiveDbUpdates.load()}, {"nrActiveDbUpdates", nrActiveDbUpdates.load()},
}; };
@ -649,8 +649,8 @@ void State::dumpStatus(Connection & conn)
if (nrStepsDone) { if (nrStepsDone) {
statusJson["totalStepTime"] = totalStepTime.load(); statusJson["totalStepTime"] = totalStepTime.load();
statusJson["totalStepBuildTime"] = totalStepBuildTime.load(); statusJson["totalStepBuildTime"] = totalStepBuildTime.load();
statusJson["avgStepTime"] = (float) totalStepTime / nrStepsDone; statusJson["avgStepTime"] = (float) totalStepTime / (float) nrStepsDone;
statusJson["avgStepBuildTime"] = (float) totalStepBuildTime / nrStepsDone; statusJson["avgStepBuildTime"] = (float) totalStepBuildTime / (float) nrStepsDone;
} }
{ {
@ -677,8 +677,8 @@ void State::dumpStatus(Connection & conn)
if (m->state->nrStepsDone) { if (m->state->nrStepsDone) {
machine["totalStepTime"] = s->totalStepTime.load(); machine["totalStepTime"] = s->totalStepTime.load();
machine["totalStepBuildTime"] = s->totalStepBuildTime.load(); machine["totalStepBuildTime"] = s->totalStepBuildTime.load();
machine["avgStepTime"] = (float) s->totalStepTime / s->nrStepsDone; machine["avgStepTime"] = (float) s->totalStepTime / (float) s->nrStepsDone;
machine["avgStepBuildTime"] = (float) s->totalStepBuildTime / s->nrStepsDone; machine["avgStepBuildTime"] = (float) s->totalStepBuildTime / (float) s->nrStepsDone;
} }
statusJson["machines"][m->sshName] = machine; statusJson["machines"][m->sshName] = machine;
} }
@ -732,11 +732,11 @@ void State::dumpStatus(Connection & conn)
{"narWriteCompressionTimeMs", stats.narWriteCompressionTimeMs.load()}, {"narWriteCompressionTimeMs", stats.narWriteCompressionTimeMs.load()},
{"narCompressionSavings", {"narCompressionSavings",
stats.narWriteBytes stats.narWriteBytes
? 1.0 - (double) stats.narWriteCompressedBytes / stats.narWriteBytes ? 1.0 - (double) stats.narWriteCompressedBytes / (double) stats.narWriteBytes
: 0.0}, : 0.0},
{"narCompressionSpeed", // MiB/s {"narCompressionSpeed", // MiB/s
stats.narWriteCompressionTimeMs 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}, : 0.0},
}; };
@ -749,20 +749,20 @@ void State::dumpStatus(Connection & conn)
{"putTimeMs", s3Stats.putTimeMs.load()}, {"putTimeMs", s3Stats.putTimeMs.load()},
{"putSpeed", {"putSpeed",
s3Stats.putTimeMs 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}, : 0.0},
{"get", s3Stats.get.load()}, {"get", s3Stats.get.load()},
{"getBytes", s3Stats.getBytes.load()}, {"getBytes", s3Stats.getBytes.load()},
{"getTimeMs", s3Stats.getTimeMs.load()}, {"getTimeMs", s3Stats.getTimeMs.load()},
{"getSpeed", {"getSpeed",
s3Stats.getTimeMs 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}, : 0.0},
{"head", s3Stats.head.load()}, {"head", s3Stats.head.load()},
{"costDollarApprox", {"costDollarApprox",
(s3Stats.get + s3Stats.head) / 10000.0 * 0.004 (double) (s3Stats.get + s3Stats.head) / 10000.0 * 0.004
+ s3Stats.put / 1000.0 * 0.005 + + (double) s3Stats.put / 1000.0 * 0.005 +
+ s3Stats.getBytes / (1024.0 * 1024.0 * 1024.0) * 0.09}, + (double) s3Stats.getBytes / (1024.0 * 1024.0 * 1024.0) * 0.09},
}; };
} }
} }

View file

@ -52,7 +52,7 @@ void State::queueMonitorLoop(Connection & conn)
auto t_after_work = std::chrono::steady_clock::now(); auto t_after_work = std::chrono::steady_clock::now();
prom.queue_monitor_time_spent_running.Increment( prom.queue_monitor_time_spent_running.Increment(
std::chrono::duration_cast<std::chrono::microseconds>(t_after_work - t_before_work).count()); static_cast<double>(std::chrono::duration_cast<std::chrono::microseconds>(t_after_work - t_before_work).count()));
/* Sleep until we get notification from the database about an /* Sleep until we get notification from the database about an
event. */ event. */
@ -79,7 +79,7 @@ void State::queueMonitorLoop(Connection & conn)
auto t_after_sleep = std::chrono::steady_clock::now(); auto t_after_sleep = std::chrono::steady_clock::now();
prom.queue_monitor_time_spent_waiting.Increment( prom.queue_monitor_time_spent_waiting.Increment(
std::chrono::duration_cast<std::chrono::microseconds>(t_after_sleep - t_after_work).count()); static_cast<double>(std::chrono::duration_cast<std::chrono::microseconds>(t_after_sleep - t_after_work).count()));
} }
exit(0); exit(0);
@ -355,7 +355,7 @@ void State::processQueueChange(Connection & conn)
pqxx::work txn(conn); pqxx::work txn(conn);
auto res = txn.exec("select id, globalPriority from Builds where finished = 0"); auto res = txn.exec("select id, globalPriority from Builds where finished = 0");
for (auto const & row : res) for (auto const & row : res)
currentIds[row["id"].as<BuildID>()] = row["globalPriority"].as<BuildID>(); currentIds[row["id"].as<BuildID>()] = row["globalPriority"].as<int>();
} }
{ {

View file

@ -98,7 +98,7 @@ public:
typedef std::shared_ptr<Jobset> ptr; typedef std::shared_ptr<Jobset> ptr;
typedef std::weak_ptr<Jobset> wptr; typedef std::weak_ptr<Jobset> wptr;
static const time_t schedulingWindow = 24 * 60 * 60; static const time_t schedulingWindow = static_cast<time_t>(24 * 60 * 60);
private: private:
@ -115,7 +115,7 @@ public:
return (double) seconds / shares; return (double) seconds / shares;
} }
void setShares(int shares_) void setShares(unsigned int shares_)
{ {
assert(shares_ > 0); assert(shares_ > 0);
shares = shares_; shares = shares_;
@ -534,7 +534,7 @@ private:
void finishBuildStep(pqxx::work & txn, const RemoteResult & result, BuildID buildId, unsigned int stepNr, void finishBuildStep(pqxx::work & txn, const RemoteResult & result, BuildID buildId, unsigned int stepNr,
const std::string & machine); 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); 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); void updateBuild(pqxx::work & txn, Build::ptr build, BuildStatus status);