From 381ce8a66658ac9d02c44e96c860cd05bcb6a5f8 Mon Sep 17 00:00:00 2001 From: Griffin Smith Date: Mon, 7 Sep 2020 13:01:49 -0400 Subject: refactor(tvix): Make static strings constexpr string_views Make all static std::strings constexpr std::string_views, and replace concatenation with absl::StrCat where necessary. Technically all of these are constant, so they really don't need to be top-level statics - and since I'm trying to get rid of as much global state as possible in preparation for making the nix daemon properly multithreaded I figured I'd knock these out while I was at it. Change-Id: Ibd3ad9ef68f0a0eacb135541b39fdb13dae042e1 Reviewed-on: https://cl.tvl.fyi/c/depot/+/1939 Tested-by: BuildkiteCI Reviewed-by: tazjin --- third_party/nix/src/libstore/binary-cache-store.cc | 2 +- third_party/nix/src/libstore/build.cc | 6 +++--- third_party/nix/src/libstore/gc.cc | 25 +++++++++++----------- third_party/nix/src/libstore/legacy-ssh-store.cc | 10 +++++---- third_party/nix/src/libstore/rpc-store.cc | 4 ++-- third_party/nix/src/libstore/ssh-store.cc | 23 +++++++++++--------- third_party/nix/src/libutil/archive.cc | 16 ++++++-------- third_party/nix/src/libutil/archive.hh | 2 +- 8 files changed, 46 insertions(+), 42 deletions(-) diff --git a/third_party/nix/src/libstore/binary-cache-store.cc b/third_party/nix/src/libstore/binary-cache-store.cc index 91da7e2265..e5b7ef2cdc 100644 --- a/third_party/nix/src/libstore/binary-cache-store.cc +++ b/third_party/nix/src/libstore/binary-cache-store.cc @@ -30,7 +30,7 @@ BinaryCacheStore::BinaryCacheStore(const Params& params) : Store(params) { } StringSink sink; - sink << narVersionMagic1; + sink << std::string(kNarVersionMagic1); narMagic = *sink.s; } diff --git a/third_party/nix/src/libstore/build.cc b/third_party/nix/src/libstore/build.cc index a009651c9d..e50f4cfa99 100644 --- a/third_party/nix/src/libstore/build.cc +++ b/third_party/nix/src/libstore/build.cc @@ -81,7 +81,7 @@ namespace nix { -static std::string pathNullDevice = "/dev/null"; +constexpr std::string_view kPathNullDevice = "/dev/null"; /* Forward definition. */ class Worker; @@ -448,9 +448,9 @@ static void commonChildInit(Pipe& logPipe) { } /* Reroute stdin to /dev/null. */ - int fdDevNull = open(pathNullDevice.c_str(), O_RDWR); + int fdDevNull = open(kPathNullDevice.begin(), O_RDWR); if (fdDevNull == -1) { - throw SysError(format("cannot open '%1%'") % pathNullDevice); + throw SysError(format("cannot open '%1%'") % kPathNullDevice); } if (dup2(fdDevNull, STDIN_FILENO) == -1) { throw SysError("cannot dup null device into stdin"); diff --git a/third_party/nix/src/libstore/gc.cc b/third_party/nix/src/libstore/gc.cc index 50c52bb009..07dc10629a 100644 --- a/third_party/nix/src/libstore/gc.cc +++ b/third_party/nix/src/libstore/gc.cc @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -22,8 +23,8 @@ namespace nix { -static std::string gcLockName = "gc.lock"; -static std::string gcRootsDir = "gcroots"; +constexpr std::string_view kGcLockName = "gc.lock"; +constexpr std::string_view kGcRootsDir = "gcroots"; /* Acquire the global GC lock. This is used to prevent new Nix processes from starting after the temporary root files have been @@ -31,7 +32,7 @@ static std::string gcRootsDir = "gcroots"; file, they will block until the garbage collector has finished / yielded the GC lock. */ AutoCloseFD LocalStore::openGCLock(LockType lockType) { - Path fnGCLock = (format("%1%/%2%") % stateDir % gcLockName).str(); + Path fnGCLock = absl::StrCat(stateDir.get(), "/", kGcLockName); DLOG(INFO) << "acquiring global GC lock " << fnGCLock; @@ -73,8 +74,8 @@ void LocalStore::syncWithGC() { AutoCloseFD fdGCLock = openGCLock(ltRead); } void LocalStore::addIndirectRoot(const Path& path) { std::string hash = hashString(htSHA1, path).to_string(Base32, false); - Path realRoot = canonPath( - (format("%1%/%2%/auto/%3%") % stateDir % gcRootsDir % hash).str()); + Path realRoot = + canonPath(absl::StrCat(stateDir.get(), "/", kGcRootsDir, "/auto/", hash)); makeSymlink(realRoot, path); } @@ -105,8 +106,7 @@ Path LocalFSStore::addPermRoot(const Path& _storePath, const Path& _gcRoot, else { if (!allowOutsideRootsDir) { - Path rootsDir = - canonPath((format("%1%/%2%") % stateDir % gcRootsDir).str()); + Path rootsDir = canonPath(absl::StrCat(stateDir.get(), "/", kGcRootsDir)); if (std::string(gcRoot, 0, rootsDir.size() + 1) != rootsDir + "/") { throw Error(format("path '%1%' is not a valid garbage collector root; " @@ -195,7 +195,7 @@ void LocalStore::addTempRoot(const Path& path) { lockFile(state->fdTempRoots.get(), ltRead, true); } -static std::string censored = "{censored}"; +constexpr std::string_view kCensored = "{censored}"; void LocalStore::findTempRoots(FDs& fds, Roots& tempRoots, bool censor) { /* Read the `temproots' directory for per-process temporary root @@ -247,7 +247,7 @@ void LocalStore::findTempRoots(FDs& fds, Roots& tempRoots, bool censor) { Path root(contents, pos, end - pos); DLOG(INFO) << "got temporary root " << root; assertStorePath(root); - tempRoots[root].emplace(censor ? censored : fmt("{temp:%d}", pid)); + tempRoots[root].emplace(censor ? kCensored : fmt("{temp:%d}", pid)); pos = end + 1; } @@ -287,7 +287,8 @@ void LocalStore::findRoots(const Path& path, unsigned char type, Roots& roots) { else { target = absPath(target, dirOf(path)); if (!pathExists(target)) { - if (isInDir(path, stateDir + "/" + gcRootsDir + "/auto")) { + if (isInDir(path, absl::StrCat(stateDir.get(), "/", kGcRootsDir, + "/auto"))) { LOG(INFO) << "removing stale link from '" << path << "' to '" << target << "'"; unlink(path.c_str()); @@ -326,7 +327,7 @@ void LocalStore::findRoots(const Path& path, unsigned char type, Roots& roots) { void LocalStore::findRootsNoTemp(Roots& roots, bool censor) { /* Process direct roots in {gcroots,profiles}. */ - findRoots(stateDir + "/" + gcRootsDir, DT_UNKNOWN, roots); + findRoots(absl::StrCat(stateDir.get(), "/", kGcRootsDir), DT_UNKNOWN, roots); findRoots(stateDir + "/profiles", DT_UNKNOWN, roots); /* Add additional roots returned by different platforms-specific @@ -465,7 +466,7 @@ void LocalStore::findRuntimeRoots(Roots& roots, bool censor) { if (isStorePath(path) && isValidPath(path)) { DLOG(INFO) << "got additional root " << path; if (censor) { - roots[path].insert(censored); + roots[path].insert(std::string(kCensored)); } else { roots[path].insert(links.begin(), links.end()); } diff --git a/third_party/nix/src/libstore/legacy-ssh-store.cc b/third_party/nix/src/libstore/legacy-ssh-store.cc index 343801870a..19603115a8 100644 --- a/third_party/nix/src/libstore/legacy-ssh-store.cc +++ b/third_party/nix/src/libstore/legacy-ssh-store.cc @@ -1,3 +1,5 @@ +#include +#include #include #include "libstore/derivations.hh" @@ -11,7 +13,7 @@ namespace nix { -static std::string uriScheme = "ssh://"; +constexpr std::string_view kUriScheme = "ssh://"; struct LegacySSHStore : public Store { const Setting maxConnections{ @@ -86,7 +88,7 @@ struct LegacySSHStore : public Store { return conn; }; - std::string getUri() override { return uriScheme + host; } + std::string getUri() override { return absl::StrCat(kUriScheme, host); } void queryPathInfoUncached( const Path& path, @@ -269,11 +271,11 @@ struct LegacySSHStore : public Store { static RegisterStoreImplementation regStore( [](const std::string& uri, const Store::Params& params) -> std::shared_ptr { - if (std::string(uri, 0, uriScheme.size()) != uriScheme) { + if (!absl::StartsWith(uri, kUriScheme)) { return nullptr; } return std::make_shared( - std::string(uri, uriScheme.size()), params); + std::string(uri, kUriScheme.size()), params); }); } // namespace nix diff --git a/third_party/nix/src/libstore/rpc-store.cc b/third_party/nix/src/libstore/rpc-store.cc index bc1fcc73d8..02975341fd 100644 --- a/third_party/nix/src/libstore/rpc-store.cc +++ b/third_party/nix/src/libstore/rpc-store.cc @@ -531,13 +531,13 @@ unsigned int RpcStore::getProtocol() { return PROTOCOL_VERSION; } } // namespace store -static std::string uriScheme = "unix://"; +constexpr std::string_view kUriScheme = "unix://"; // TODO(grfn): Make this a function that we call from main rather than... this static RegisterStoreImplementation regStore([](const std::string& uri, const Store::Params& params) -> std::shared_ptr { - if (std::string(uri, 0, uriScheme.size()) != uriScheme) { + if (std::string(uri, 0, kUriScheme.size()) != kUriScheme) { return nullptr; } auto channel = grpc::CreateChannel(uri, grpc::InsecureChannelCredentials()); diff --git a/third_party/nix/src/libstore/ssh-store.cc b/third_party/nix/src/libstore/ssh-store.cc index 48fea858a3..96adb3660d 100644 --- a/third_party/nix/src/libstore/ssh-store.cc +++ b/third_party/nix/src/libstore/ssh-store.cc @@ -1,3 +1,5 @@ +#include + #include "libstore/remote-fs-accessor.hh" #include "libstore/remote-store.hh" #include "libstore/ssh.hh" @@ -8,7 +10,7 @@ namespace nix { -static std::string uriScheme = "ssh-ng://"; +constexpr std::string_view kUriScheme = "ssh-ng://"; class SSHStore : public RemoteStore { public: @@ -25,7 +27,7 @@ class SSHStore : public RemoteStore { // Use SSH master only if using more than 1 connection. connections->capacity() > 1, compress) {} - std::string getUri() override { return uriScheme + host; } + std::string getUri() override { return absl::StrCat(kUriScheme, host); } bool sameMachine() override { return false; } @@ -74,13 +76,14 @@ ref SSHStore::openConnection() { return conn; } -static RegisterStoreImplementation regStore([](const std::string& uri, - const Store::Params& params) - -> std::shared_ptr { - if (std::string(uri, 0, uriScheme.size()) != uriScheme) { - return nullptr; - } - return std::make_shared(std::string(uri, uriScheme.size()), params); -}); +static RegisterStoreImplementation regStore( + [](const std::string& uri, + const Store::Params& params) -> std::shared_ptr { + if (std::string(uri, 0, kUriScheme.size()) != kUriScheme) { + return nullptr; + } + return std::make_shared(std::string(uri, kUriScheme.size()), + params); + }); } // namespace nix diff --git a/third_party/nix/src/libutil/archive.cc b/third_party/nix/src/libutil/archive.cc index e3233d9ee4..e470ad7be6 100644 --- a/third_party/nix/src/libutil/archive.cc +++ b/third_party/nix/src/libutil/archive.cc @@ -36,9 +36,7 @@ static ArchiveSettings archiveSettings; static GlobalConfig::Register r1(&archiveSettings); -const std::string narVersionMagic1 = "nix-archive-1"; - -static std::string caseHackSuffix = "~nix~case~hack~"; +constexpr std::string_view kCaseHackSuffix = "~nix~case~hack~"; PathFilter defaultPathFilter = [](const Path& /*unused*/) { return true; }; @@ -93,7 +91,7 @@ static void dump(const Path& path, Sink& sink, PathFilter& filter) { for (auto& i : readDirectory(path)) { if (archiveSettings.useCaseHack) { std::string name(i.name); - size_t pos = i.name.find(caseHackSuffix); + size_t pos = i.name.find(kCaseHackSuffix); if (pos != std::string::npos) { DLOG(INFO) << "removing case hack suffix from " << path << "/" << i.name; @@ -134,12 +132,12 @@ static void dump(const Path& path, Sink& sink, PathFilter& filter) { } void dumpPath(const Path& path, Sink& sink, PathFilter& filter) { - sink << narVersionMagic1; + sink << std::string(kNarVersionMagic1); dump(path, sink, filter); } void dumpString(const std::string& s, Sink& sink) { - sink << narVersionMagic1 << "(" + sink << std::string(kNarVersionMagic1) << "(" << "type" << "regular" << "contents" << s << ")"; @@ -279,7 +277,7 @@ static void parse(ParseSink& sink, Source& source, const Path& path) { if (i != names.end()) { DLOG(INFO) << "case collision between '" << i->first << "' and '" << name << "'"; - name += caseHackSuffix; + name += kCaseHackSuffix; name += std::to_string(++i->second); } else { names[name] = 0; @@ -310,12 +308,12 @@ static void parse(ParseSink& sink, Source& source, const Path& path) { void parseDump(ParseSink& sink, Source& source) { std::string version; try { - version = readString(source, narVersionMagic1.size()); + version = readString(source, kNarVersionMagic1.size()); } catch (SerialisationError& e) { /* This generally means the integer at the start couldn't be decoded. Ignore and throw the exception below. */ } - if (version != narVersionMagic1) { + if (version != kNarVersionMagic1) { throw badArchive("input doesn't look like a Nix archive"); } parse(sink, source, ""); diff --git a/third_party/nix/src/libutil/archive.hh b/third_party/nix/src/libutil/archive.hh index 194d31d078..3966278785 100644 --- a/third_party/nix/src/libutil/archive.hh +++ b/third_party/nix/src/libutil/archive.hh @@ -72,6 +72,6 @@ void restorePath(const Path& path, Source& source); /* Read a NAR from 'source' and write it to 'sink'. */ void copyNAR(Source& source, Sink& sink); -extern const std::string narVersionMagic1; +constexpr std::string_view kNarVersionMagic1 = "nix-archive-1"; } // namespace nix -- cgit 1.4.1