refactor(3p/nix): Apply clang-tidy's performance-* fixes

This applies the performance fixes listed here:

https://clang.llvm.org/extra/clang-tidy/checks/list.html
This commit is contained in:
Vincent Ambo 2020-05-20 22:58:43 +01:00
parent 689ef502f5
commit 43677021e3
60 changed files with 189 additions and 166 deletions

View file

@ -98,7 +98,7 @@ Path BinaryCacheStore::narInfoFileFor(const Path& storePath) {
return storePathToHash(storePath) + ".narinfo";
}
void BinaryCacheStore::writeNarInfo(ref<NarInfo> narInfo) {
void BinaryCacheStore::writeNarInfo(const ref<NarInfo>& narInfo) {
auto narInfoFile = narInfoFileFor(narInfo->path);
upsertFile(narInfoFile, narInfo->to_string(), "text/x-nix-narinfo");

View file

@ -65,7 +65,7 @@ class BinaryCacheStore : public Store {
std::string narInfoFileFor(const Path& storePath);
void writeNarInfo(ref<NarInfo> narInfo);
void writeNarInfo(const ref<NarInfo>& narInfo);
public:
bool isValidPathUncached(const Path& path) override;

View file

@ -145,7 +145,7 @@ class Goal : public std::enable_shared_from_this<Goal> {
public:
virtual void work() = 0;
void addWaitee(GoalPtr waitee);
void addWaitee(const GoalPtr& waitee);
virtual void waiteeDone(GoalPtr waitee, ExitCode result);
@ -280,10 +280,10 @@ class Worker {
RepairFlag repair = NoRepair);
/* Remove a dead goal. */
void removeGoal(GoalPtr goal);
void removeGoal(const GoalPtr& goal);
/* Wake up a goal (i.e., there is something for it to do). */
void wakeUp(GoalPtr goal);
void wakeUp(const GoalPtr& goal);
/* Return the number of local build and substitution processes
currently running (but not remote builds via the build
@ -292,7 +292,7 @@ class Worker {
/* Registers a running child process. `inBuildSlot' means that
the process counts towards the jobs limit. */
void childStarted(GoalPtr goal, const set<int>& fds, bool inBuildSlot,
void childStarted(const GoalPtr& goal, const set<int>& fds, bool inBuildSlot,
bool respectTimeouts);
/* Unregisters a running child process. `wakeSleepers' should be
@ -303,7 +303,7 @@ class Worker {
/* Put `goal' to sleep until a build slot becomes available (which
might be right away). */
void waitForBuildSlot(GoalPtr goal);
void waitForBuildSlot(const GoalPtr& goal);
/* Wait for any goal to finish. Pretty indiscriminate way to
wait for some resource that some other goal is holding. */
@ -332,7 +332,7 @@ class Worker {
//////////////////////////////////////////////////////////////////////
void addToWeakGoals(WeakGoals& goals, GoalPtr p) {
void addToWeakGoals(WeakGoals& goals, const GoalPtr& p) {
// FIXME: necessary?
// FIXME: O(n)
for (auto& i : goals) {
@ -343,7 +343,7 @@ void addToWeakGoals(WeakGoals& goals, GoalPtr p) {
goals.push_back(p);
}
void Goal::addWaitee(GoalPtr waitee) {
void Goal::addWaitee(const GoalPtr& waitee) {
waitees.insert(waitee);
addToWeakGoals(waitee->waiters, shared_from_this());
}
@ -445,7 +445,9 @@ void handleDiffHook(uid_t uid, uid_t gid, Path tryA, Path tryB, Path drvPath,
auto diffHook = settings.diffHook;
if (diffHook != "" && settings.runDiffHook) {
try {
RunOptions diffHookOptions(diffHook, {tryA, tryB, drvPath, tmpDir});
RunOptions diffHookOptions(
diffHook, {std::move(tryA), std::move(tryB), std::move(drvPath),
std::move(tmpDir)});
diffHookOptions.searchPath = true;
diffHookOptions.uid = uid;
diffHookOptions.gid = gid;
@ -979,7 +981,7 @@ class DerivationGoal : public Goal {
void done(BuildResult::Status status, const string& msg = "");
PathSet exportReferences(PathSet storePaths);
PathSet exportReferences(const PathSet& storePaths);
};
const Path DerivationGoal::homeDir = "/homeless-shelter";
@ -1520,7 +1522,7 @@ void DerivationGoal::tryToBuild() {
started();
}
void replaceValidPath(const Path& storePath, const Path tmpPath) {
void replaceValidPath(const Path& storePath, const Path& tmpPath) {
/* We can't atomically replace storePath (the original) with
tmpPath (the replacement), so we have to move it out of the
way first. We'd better not be interrupted here, because if
@ -1840,7 +1842,7 @@ int childEntry(void* arg) {
return 1;
}
PathSet DerivationGoal::exportReferences(PathSet storePaths) {
PathSet DerivationGoal::exportReferences(const PathSet& storePaths) {
PathSet paths;
for (auto storePath : storePaths) {
@ -4455,7 +4457,7 @@ GoalPtr Worker::makeSubstitutionGoal(const Path& path, RepairFlag repair) {
return goal;
}
static void removeGoal(GoalPtr goal, WeakGoalMap& goalMap) {
static void removeGoal(const GoalPtr& goal, WeakGoalMap& goalMap) {
/* !!! inefficient */
for (auto i = goalMap.begin(); i != goalMap.end();) {
if (i->second.lock() == goal) {
@ -4469,7 +4471,7 @@ static void removeGoal(GoalPtr goal, WeakGoalMap& goalMap) {
}
}
void Worker::removeGoal(GoalPtr goal) {
void Worker::removeGoal(const GoalPtr& goal) {
nix::removeGoal(goal, derivationGoals);
nix::removeGoal(goal, substitutionGoals);
if (topGoals.find(goal) != topGoals.end()) {
@ -4492,15 +4494,15 @@ void Worker::removeGoal(GoalPtr goal) {
waitingForAnyGoal.clear();
}
void Worker::wakeUp(GoalPtr goal) {
void Worker::wakeUp(const GoalPtr& goal) {
goal->trace("woken up");
addToWeakGoals(awake, goal);
}
unsigned Worker::getNrLocalBuilds() { return nrLocalBuilds; }
void Worker::childStarted(GoalPtr goal, const set<int>& fds, bool inBuildSlot,
bool respectTimeouts) {
void Worker::childStarted(const GoalPtr& goal, const set<int>& fds,
bool inBuildSlot, bool respectTimeouts) {
Child child;
child.goal = goal;
child.goal2 = goal.get();
@ -4542,7 +4544,7 @@ void Worker::childTerminated(Goal* goal, bool wakeSleepers) {
}
}
void Worker::waitForBuildSlot(GoalPtr goal) {
void Worker::waitForBuildSlot(const GoalPtr& goal) {
DLOG(INFO) << "wait for build slot";
if (getNrLocalBuilds() < settings.maxBuildJobs) {
wakeUp(goal); /* we can do it right away */
@ -4553,12 +4555,12 @@ void Worker::waitForBuildSlot(GoalPtr goal) {
void Worker::waitForAnyGoal(GoalPtr goal) {
DLOG(INFO) << "wait for any goal";
addToWeakGoals(waitingForAnyGoal, goal);
addToWeakGoals(waitingForAnyGoal, std::move(goal));
}
void Worker::waitForAWhile(GoalPtr goal) {
DLOG(INFO) << "wait for a while";
addToWeakGoals(waitingForAWhile, goal);
addToWeakGoals(waitingForAWhile, std::move(goal));
}
void Worker::run(const Goals& _topGoals) {

View file

@ -104,12 +104,12 @@ PublicKeys getDefaultPublicKeys() {
// FIXME: filter duplicates
for (auto s : settings.trustedPublicKeys.get()) {
for (const auto& s : settings.trustedPublicKeys.get()) {
PublicKey key(s);
publicKeys.emplace(key.name, key);
}
for (auto secretKeyFile : settings.secretKeyFiles.get()) {
for (const auto& secretKeyFile : settings.secretKeyFiles.get()) {
try {
SecretKey secretKey(readFile(secretKeyFile));
publicKeys.emplace(secretKey.name, secretKey.toPublicKey());

View file

@ -38,7 +38,7 @@ bool BasicDerivation::isBuiltin() const {
return string(builder, 0, 8) == "builtin:";
}
Path writeDerivation(ref<Store> store, const Derivation& drv,
Path writeDerivation(const ref<Store>& store, const Derivation& drv,
const string& name, RepairFlag repair) {
PathSet references;
references.insert(drv.inputSrcs.begin(), drv.inputSrcs.end());
@ -355,7 +355,7 @@ Hash hashDerivationModulo(Store& store, Derivation drv) {
}
DrvPathWithOutputs parseDrvPathWithOutputs(const string& s) {
size_t n = s.find("!");
size_t n = s.find('!');
return n == std::string::npos
? DrvPathWithOutputs(s, std::set<string>())
: DrvPathWithOutputs(

View file

@ -67,7 +67,7 @@ struct Derivation : BasicDerivation {
class Store;
/* Write a derivation to the Nix store, and return its path. */
Path writeDerivation(ref<Store> store, const Derivation& drv,
Path writeDerivation(const ref<Store>& store, const Derivation& drv,
const string& name, RepairFlag repair = NoRepair);
/* Read a derivation from a file. */

View file

@ -130,7 +130,7 @@ struct CurlDownloader : public Downloader {
}
}
void failEx(std::exception_ptr ex) {
void failEx(const std::exception_ptr& ex) {
assert(!done);
done = true;
callback.rethrow(ex);
@ -663,7 +663,7 @@ struct CurlDownloader : public Downloader {
}
}
void enqueueItem(std::shared_ptr<DownloadItem> item) {
void enqueueItem(const std::shared_ptr<DownloadItem>& item) {
if (item->request.data && !hasPrefix(item->request.uri, "http://") &&
!hasPrefix(item->request.uri, "https://")) {
throw nix::Error("uploading to '%s' is not supported", item->request.uri);
@ -858,7 +858,7 @@ void Downloader::download(DownloadRequest&& request, Sink& sink) {
}
CachedDownloadResult Downloader::downloadCached(
ref<Store> store, const CachedDownloadRequest& request) {
const ref<Store>& store, const CachedDownloadRequest& request) {
auto url = resolveUri(request.uri);
auto name = request.name;

View file

@ -108,7 +108,7 @@ struct Downloader {
and is more recent than tarball-ttl seconds. Otherwise,
use the recorded ETag to verify if the server has a more
recent version, and if so, download it to the Nix store. */
CachedDownloadResult downloadCached(ref<Store> store,
CachedDownloadResult downloadCached(const ref<Store>& store,
const CachedDownloadRequest& request);
enum Error { NotFound, Forbidden, Misc, Transient, Interrupted };

View file

@ -55,7 +55,8 @@ void Store::exportPath(const Path& path, Sink& sink) {
<< 0;
}
Paths Store::importPaths(Source& source, std::shared_ptr<FSAccessor> accessor,
Paths Store::importPaths(Source& source,
const std::shared_ptr<FSAccessor>& accessor,
CheckSigsFlag checkSigs) {
Paths res;
while (true) {

View file

@ -149,17 +149,18 @@ void BaseSetting<SandboxMode>::convertToArg(Args& args,
args.mkFlag()
.longName(name)
.description("Enable sandboxing.")
.handler([=](std::vector<std::string> ss) { override(smEnabled); })
.handler([=](const std::vector<std::string>& ss) { override(smEnabled); })
.category(category);
args.mkFlag()
.longName("no-" + name)
.description("Disable sandboxing.")
.handler([=](std::vector<std::string> ss) { override(smDisabled); })
.handler(
[=](const std::vector<std::string>& ss) { override(smDisabled); })
.category(category);
args.mkFlag()
.longName("relaxed-" + name)
.description("Enable sandboxing, but allow builds to disable it.")
.handler([=](std::vector<std::string> ss) { override(smRelaxed); })
.handler([=](const std::vector<std::string>& ss) { override(smRelaxed); })
.category(category);
}

View file

@ -12,7 +12,7 @@ LocalFSStore::LocalFSStore(const Params& params) : Store(params) {}
struct LocalStoreAccessor : public FSAccessor {
ref<LocalFSStore> store;
explicit LocalStoreAccessor(ref<LocalFSStore> store) : store(store) {}
explicit LocalStoreAccessor(const ref<LocalFSStore>& store) : store(store) {}
Path toRealPath(const Path& path) {
Path storePath = store->toStorePath(path);

View file

@ -9,12 +9,12 @@
namespace nix {
Machine::Machine(decltype(storeUri) storeUri, decltype(systemTypes) systemTypes,
decltype(sshKey) sshKey, decltype(maxJobs) maxJobs,
decltype(speedFactor) speedFactor,
decltype(supportedFeatures) supportedFeatures,
decltype(mandatoryFeatures) mandatoryFeatures,
decltype(sshPublicHostKey) sshPublicHostKey)
Machine::Machine(decltype(storeUri)& storeUri,
decltype(systemTypes)& systemTypes, decltype(sshKey)& sshKey,
decltype(maxJobs) maxJobs, decltype(speedFactor) speedFactor,
decltype(supportedFeatures)& supportedFeatures,
decltype(mandatoryFeatures)& mandatoryFeatures,
decltype(sshPublicHostKey)& sshPublicHostKey)
: storeUri(
// Backwards compatibility: if the URI is a hostname,
// prepend ssh://.

View file

@ -19,12 +19,12 @@ struct Machine {
bool mandatoryMet(const std::set<string>& features) const;
Machine(decltype(storeUri) storeUri, decltype(systemTypes) systemTypes,
decltype(sshKey) sshKey, decltype(maxJobs) maxJobs,
Machine(decltype(storeUri)& storeUri, decltype(systemTypes)& systemTypes,
decltype(sshKey)& sshKey, decltype(maxJobs) maxJobs,
decltype(speedFactor) speedFactor,
decltype(supportedFeatures) supportedFeatures,
decltype(mandatoryFeatures) mandatoryFeatures,
decltype(sshPublicHostKey) sshPublicHostKey);
decltype(supportedFeatures)& supportedFeatures,
decltype(mandatoryFeatures)& mandatoryFeatures,
decltype(sshPublicHostKey)& sshPublicHostKey);
};
typedef std::vector<Machine> Machines;

View file

@ -171,8 +171,9 @@ void Store::queryMissing(const PathSet& targets, PathSet& willBuild_,
}
};
auto checkOutput = [&](const Path& drvPath, ref<Derivation> drv,
const Path& outPath, ref<Sync<DrvState>> drvState_) {
auto checkOutput = [&](const Path& drvPath, const ref<Derivation>& drv,
const Path& outPath,
const ref<Sync<DrvState>>& drvState_) {
if (drvState_->lock()->done) {
return;
}

View file

@ -94,7 +94,7 @@ struct NarAccessor : public FSAccessor {
}
};
explicit NarAccessor(ref<const std::string> nar) : nar(nar) {
explicit NarAccessor(const ref<const std::string>& nar) : nar(nar) {
NarIndexer indexer(*this, *nar);
parseDump(indexer, indexer);
}
@ -111,7 +111,7 @@ struct NarAccessor : public FSAccessor {
if (type == "directory") {
member.type = FSAccessor::Type::tDirectory;
for (auto i = v["entries"].begin(); i != v["entries"].end(); ++i) {
std::string name = i.key();
const std::string& name = i.key();
recurse(member.children[name], i.value());
}
} else if (type == "regular") {
@ -225,8 +225,8 @@ ref<FSAccessor> makeLazyNarAccessor(const std::string& listing,
return make_ref<NarAccessor>(listing, getNarBytes);
}
void listNar(JSONPlaceholder& res, ref<FSAccessor> accessor, const Path& path,
bool recurse) {
void listNar(JSONPlaceholder& res, const ref<FSAccessor>& accessor,
const Path& path, bool recurse) {
auto st = accessor->stat(path);
auto obj = res.object();

View file

@ -23,7 +23,7 @@ class JSONPlaceholder;
/* Write a JSON representation of the contents of a NAR (except file
contents). */
void listNar(JSONPlaceholder& res, ref<FSAccessor> accessor, const Path& path,
bool recurse);
void listNar(JSONPlaceholder& res, const ref<FSAccessor>& accessor,
const Path& path, bool recurse);
} // namespace nix

View file

@ -122,7 +122,7 @@ std::string NarInfo::to_string() const {
res += "System: " + system + "\n";
}
for (auto sig : sigs) {
for (const auto& sig : sigs) {
res += "Sig: " + sig + "\n";
}

View file

@ -35,7 +35,7 @@ static int parseName(const string& profileName, const string& name) {
return -1;
}
Generations findGenerations(Path profile, int& curGen) {
Generations findGenerations(const Path& profile, int& curGen) {
Generations gens;
Path profileDir = dirOf(profile);
@ -68,7 +68,8 @@ static void makeName(const Path& profile, unsigned int num, Path& outLink) {
outLink = prefix + "-link";
}
Path createGeneration(ref<LocalFSStore> store, Path profile, Path outPath) {
Path createGeneration(const ref<LocalFSStore>& store, const Path& profile,
const Path& outPath) {
/* The new generation number should be higher than old the
previous ones. */
int dummy;
@ -226,7 +227,7 @@ void deleteGenerationsOlderThan(const Path& profile, const string& timeSpec,
deleteGenerationsOlderThan(profile, oldTime, dryRun);
}
void switchLink(Path link, Path target) {
void switchLink(const Path& link, Path target) {
/* Hacky. */
if (dirOf(target) == dirOf(link)) {
target = baseNameOf(target);

View file

@ -19,11 +19,12 @@ typedef list<Generation> Generations;
/* Returns the list of currently present generations for the specified
profile, sorted by generation number. */
Generations findGenerations(Path profile, int& curGen);
Generations findGenerations(const Path& profile, int& curGen);
class LocalFSStore;
Path createGeneration(ref<LocalFSStore> store, Path profile, Path outPath);
Path createGeneration(const ref<LocalFSStore>& store, const Path& profile,
const Path& outPath);
void deleteGeneration(const Path& profile, unsigned int gen);
@ -40,7 +41,7 @@ void deleteGenerationsOlderThan(const Path& profile, time_t t, bool dryRun);
void deleteGenerationsOlderThan(const Path& profile, const string& timeSpec,
bool dryRun);
void switchLink(Path link, Path target);
void switchLink(const Path& link, Path target);
/* Ensure exclusive access to a profile. Any command that modifies
the profile first acquires this lock. */

View file

@ -9,7 +9,8 @@
namespace nix {
RemoteFSAccessor::RemoteFSAccessor(ref<Store> store, const Path& cacheDir)
RemoteFSAccessor::RemoteFSAccessor(const ref<Store>& store,
const Path& cacheDir)
: store(store), cacheDir(cacheDir) {
if (!cacheDir.empty()) {
createDirs(cacheDir);
@ -23,7 +24,7 @@ Path RemoteFSAccessor::makeCacheFile(const Path& storePath,
}
void RemoteFSAccessor::addToCache(const Path& storePath, const std::string& nar,
ref<FSAccessor> narAccessor) {
const ref<FSAccessor>& narAccessor) {
nars.emplace(storePath, narAccessor);
if (!cacheDir.empty()) {

View file

@ -20,10 +20,10 @@ class RemoteFSAccessor : public FSAccessor {
Path makeCacheFile(const Path& storePath, const std::string& ext);
void addToCache(const Path& storePath, const std::string& nar,
ref<FSAccessor> narAccessor);
const ref<FSAccessor>& narAccessor);
public:
RemoteFSAccessor(ref<Store> store,
RemoteFSAccessor(const ref<Store>& store,
const /* FIXME: use std::optional */ Path& cacheDir = "");
Stat stat(const Path& path) override;

View file

@ -1,6 +1,7 @@
#include "store-api.hh"
#include <future>
#include <utility>
#include <glog/logging.h>
@ -570,7 +571,7 @@ void Store::buildPaths(const PathSet& paths, BuildMode buildMode) {
}
}
void copyStorePath(ref<Store> srcStore, ref<Store> dstStore,
void copyStorePath(ref<Store> srcStore, const ref<Store>& dstStore,
const Path& storePath, RepairFlag repair,
CheckSigsFlag checkSigs) {
auto srcUri = srcStore->getUri();
@ -693,7 +694,7 @@ void copyPaths(ref<Store> srcStore, ref<Store> dstStore,
});
}
void copyClosure(ref<Store> srcStore, ref<Store> dstStore,
void copyClosure(const ref<Store>& srcStore, const ref<Store>& dstStore,
const PathSet& storePaths, RepairFlag repair,
CheckSigsFlag checkSigs, SubstituteFlag substitute) {
PathSet closure;
@ -824,14 +825,14 @@ void Store::addToStore(const ValidPathInfo& info, Source& narSource,
RepairFlag repair, CheckSigsFlag checkSigs,
std::shared_ptr<FSAccessor> accessor) {
addToStore(info, make_ref<std::string>(narSource.drain()), repair, checkSigs,
accessor);
std::move(accessor));
}
void Store::addToStore(const ValidPathInfo& info, const ref<std::string>& nar,
RepairFlag repair, CheckSigsFlag checkSigs,
std::shared_ptr<FSAccessor> accessor) {
StringSource source(*nar);
addToStore(info, source, repair, checkSigs, accessor);
addToStore(info, source, repair, checkSigs, std::move(accessor));
}
} // namespace nix
@ -851,7 +852,7 @@ std::pair<std::string, Store::Params> splitUriAndParams(
Store::Params params;
auto q = uri.find('?');
if (q != std::string::npos) {
for (auto s : tokenizeString<Strings>(uri.substr(q + 1), "&")) {
for (const auto& s : tokenizeString<Strings>(uri.substr(q + 1), "&")) {
auto e = s.find('=');
if (e != std::string::npos) {
auto value = s.substr(e + 1);
@ -885,7 +886,7 @@ ref<Store> openStore(const std::string& uri_,
auto params = extraParams;
params.insert(uriParams.begin(), uriParams.end());
for (auto fun : *RegisterStoreImplementation::implementations) {
for (const auto& fun : *RegisterStoreImplementation::implementations) {
auto store = fun(uri, params);
if (store) {
store->warnUnknownSettings();
@ -952,11 +953,11 @@ std::list<ref<Store>> getDefaultSubstituters() {
}
};
for (auto uri : settings.substituters.get()) {
for (const auto& uri : settings.substituters.get()) {
addStore(uri);
}
for (auto uri : settings.extraSubstituters.get()) {
for (const auto& uri : settings.extraSubstituters.get()) {
addStore(uri);
}

View file

@ -564,7 +564,7 @@ class Store : public std::enable_shared_from_this<Store>, public Config {
the Nix store. Optionally, the contents of the NARs are
preloaded into the specified FS accessor to speed up subsequent
access. */
Paths importPaths(Source& source, std::shared_ptr<FSAccessor> accessor,
Paths importPaths(Source& source, const std::shared_ptr<FSAccessor>& accessor,
CheckSigsFlag checkSigs = CheckSigs);
struct Stats {
@ -671,7 +671,7 @@ string storePathToHash(const Path& path);
void checkStoreName(const string& name);
/* Copy a path from one store to another. */
void copyStorePath(ref<Store> srcStore, ref<Store> dstStore,
void copyStorePath(ref<Store> srcStore, const ref<Store>& dstStore,
const Path& storePath, RepairFlag repair = NoRepair,
CheckSigsFlag checkSigs = CheckSigs);
@ -686,7 +686,7 @@ void copyPaths(ref<Store> srcStore, ref<Store> dstStore,
SubstituteFlag substitute = NoSubstitute);
/* Copy the closure of the specified paths from one store to another. */
void copyClosure(ref<Store> srcStore, ref<Store> dstStore,
void copyClosure(const ref<Store>& srcStore, const ref<Store>& dstStore,
const PathSet& storePaths, RepairFlag repair = NoRepair,
CheckSigsFlag checkSigs = CheckSigs,
SubstituteFlag substitute = NoSubstitute);