Enable more clang-tidy lints

.clang-tidy

@@ -1,7 +1,23 @@
 Checks:
-  - -*
   - bugprone-*
   - performance-*
   - modernize-*
-  - cppcoreguidelines-pro-type-cstyle-cast
+  - readability-*
+  - misc-*
+  - portability-*
+  - concurrency-*
+  - google-*
+  - -google-readability-todo
+
+  # don't find them too problematic
+  - -readability-identifier-length
+  - -readability-magic-numbers
+
+  # maybe address this in the future
+  - -readability-function-cognitive-complexity
+
+  - cppcoreguidelines-*
+  - -cppcoreguidelines-avoid-magic-numbers
 UseColor: true
+CheckOptions:
+  misc-non-private-member-variables-in-classes.IgnoreClassesWithAllMemberVariablesBeingPublic: True

shell.nix

@@ -24,20 +24,10 @@ let
 in
 (pkgs.mkShell.override { inherit stdenv; }) {
   inherit (nix-eval-jobs) buildInputs;
-  nativeBuildInputs =
-    nix-eval-jobs.nativeBuildInputs
-    ++ [ (pkgs.python3.withPackages (ps: [ ps.pytest ])) ]
-    ++
-      lib.optional stdenv.isLinux # broken on darwin
-        (
-          pkgs.writeShellScriptBin "update-include-what-you-use" ''
-            #!${pkgs.stdenv.shell}
-            export PATH=${pkgs.include-what-you-use}/bin:$PATH
-            find src -type f -name '*.cpp' -o -name '*.hh' -print0 | \
-              xargs -n1 --null include-what-you-use -std=c++20 -isystem ${lib.getDev nix}/include/nix 2>&1 | \
-              fix_includes.py
-          ''
-        );
+  nativeBuildInputs = nix-eval-jobs.nativeBuildInputs ++ [
+    (pkgs.python3.withPackages (ps: [ ps.pytest ]))
+    (lib.hiPrio pkgs.clang-tools)
+  ];
 
   shellHook = lib.optionalString stdenv.isLinux ''
     export NIX_DEBUG_INFO_DIRS="${pkgs.curl.debug}/lib/debug:${nix.debug}/lib/debug''${NIX_DEBUG_INFO_DIRS:+:$NIX_DEBUG_INFO_DIRS}"

src/buffered-io.cc

@@ -2,19 +2,27 @@
 #include <unistd.h>
 #include <cerrno>
 #include <cstdlib>
-#include <sys/types.h>
+// NOLINTBEGIN(modernize-deprecated-headers)
+// misc-include-cleaner wants these headers rather than the C++ version
+#include <stdio.h>
+#include <string.h>
+// NOLINTEND(modernize-deprecated-headers)
+#include <cstdio>
 #include <nix/error.hh>
 #include <nix/signals.hh>
 #include <nix/signals-impl.hh>
+#include <string>
+#include <string_view>
 
 #include "buffered-io.hh"
+#include "strings-portable.hh"
 
 [[nodiscard]] auto tryWriteLine(int fd, std::string s) -> int {
     s += "\n";
     std::string_view sv{s};
     while (!sv.empty()) {
         nix::checkInterrupt();
-        ssize_t res = write(fd, sv.data(), sv.size());
+        const ssize_t res = write(fd, sv.data(), sv.size());
         if (res == -1 && errno != EINTR) {
             return -errno;
         }
@@ -25,29 +33,23 @@
     return 0;
 }
 
-LineReader::LineReader(int fd) {
-    stream = fdopen(fd, "r");
-    if (!stream) {
-        throw nix::Error("fdopen(%d) failed: %s", fd, strerror(errno));
+LineReader::LineReader(int fd) : stream(fdopen(fd, "r")) {
+    if (stream == nullptr) {
+        throw nix::Error("fdopen(%d) failed: %s", fd, get_error_name(errno));
     }
 }
 
-LineReader::~LineReader() {
-    fclose(stream);
-    free(buffer);
-}
-
-LineReader::LineReader(LineReader &&other) noexcept {
-    stream = other.stream;
+LineReader::LineReader(LineReader &&other) noexcept
+    : stream(other.stream.release()), buffer(other.buffer.release()),
+      len(other.len) {
     other.stream = nullptr;
-    buffer = other.buffer;
-    other.buffer = nullptr;
-    len = other.len;
     other.len = 0;
 }
 
 [[nodiscard]] auto LineReader::readLine() -> std::string_view {
-    ssize_t read = getline(&buffer, &len, stream);
+    char *buf = buffer.release();
+    const ssize_t read = getline(&buf, &len, stream.get());
+    buffer.reset(buf);
 
     if (read == -1) {
         return {}; // Return an empty string_view in case of error
@@ -56,5 +58,6 @@ LineReader::LineReader(LineReader &&other) noexcept {
     nix::checkInterrupt();
 
     // Remove trailing newline
-    return {buffer, static_cast<size_t>(read) - 1};
+    char *line = buffer.get();
+    return {line, static_cast<size_t>(read) - 1};
 }

src/buffered-io.hh

@@ -2,19 +2,40 @@
 #include <cstdio>
 #include <string>
 #include <string_view>
+#include <memory>
+#include <cstdlib>
 
 [[nodiscard]] auto tryWriteLine(int fd, std::string s) -> int;
 
+struct FileDeleter {
+    void operator()(FILE *file) const {
+        if (file != nullptr) {
+            std::fclose(file); // NOLINT(cppcoreguidelines-owning-memory)
+        }
+    }
+};
+
+struct MemoryDeleter {
+    void operator()(void *ptr) const {
+        // NOLINTBEGIN(cppcoreguidelines-owning-memory,cppcoreguidelines-no-malloc)
+        std::free(ptr);
+        // NOLINTEND(cppcoreguidelines-owning-memory,cppcoreguidelines-no-malloc)
+    }
+};
+
 class LineReader {
   public:
-    LineReader(int fd);
-    ~LineReader();
+    LineReader(const LineReader &) = delete;
+    explicit LineReader(int fd);
+    auto operator=(const LineReader &) -> LineReader & = delete;
+    auto operator=(LineReader &&) -> LineReader & = delete;
+    ~LineReader() = default;
 
     LineReader(LineReader &&other) noexcept;
     [[nodiscard]] auto readLine() -> std::string_view;
 
   private:
-    FILE *stream = nullptr;
-    char *buffer = nullptr;
+    std::unique_ptr<FILE, FileDeleter> stream = nullptr;
+    std::unique_ptr<char, MemoryDeleter> buffer = nullptr;
     size_t len = 0;
 };

src/drv.cc

@@ -3,11 +3,11 @@
 #include <nix/store-api.hh>
 #include <nix/local-fs-store.hh>
 #include <nix/value-to-json.hh>
+#include <nix/config.hh>
 #include <nix/derivations.hh>
 #include <nix/get-drvs.hh>
 #include <nix/derived-path-map.hh>
 #include <nix/eval.hh>
-#include <nlohmann/detail/json_ref.hpp>
 #include <nlohmann/json.hpp>
 #include <nlohmann/json_fwd.hpp>
 #include <nix/path.hh>
@@ -17,20 +17,28 @@
 #include <nix/eval-error.hh>
 #include <nix/experimental-features.hh>
 #include <nix/pos-idx.hh>
+#include <cstdint>
+#include <string>
 #include <exception>
 #include <sstream>
 #include <vector>
-#include <memory>
+#include <optional>
+#include <map>
+#include <set>
 
 #include "drv.hh"
 #include "eval-args.hh"
 
-static auto
-queryCacheStatus(nix::Store &store,
-                 std::map<std::string, std::optional<std::string>> &outputs)
-    -> Drv::CacheStatus {
-    uint64_t downloadSize, narSize;
-    nix::StorePathSet willBuild, willSubstitute, unknown;
+namespace {
+
+auto queryCacheStatus(nix::Store &store,
+                      std::map<std::string, std::optional<std::string>>
+                          &outputs) -> Drv::CacheStatus {
+    uint64_t downloadSize = 0;
+    uint64_t narSize = 0;
+    nix::StorePathSet willBuild;
+    nix::StorePathSet willSubstitute;
+    nix::StorePathSet unknown;
 
     std::vector<nix::StorePathWithOutputs> paths;
     for (auto const &[key, val] : outputs) {
@@ -47,16 +55,15 @@ queryCacheStatus(nix::Store &store,
             //  - there's nothing to build
             //  - there's nothing to substitute
             return Drv::CacheStatus::Local;
-        } else {
-            // cacheStatus is Cached if:
-            //  - there's nothing to build
-            //  - there are paths to substitute
-            return Drv::CacheStatus::Cached;
         }
-    } else {
-        return Drv::CacheStatus::NotBuilt;
+        // cacheStatus is Cached if:
+        //  - there's nothing to build
+        //  - there are paths to substitute
+        return Drv::CacheStatus::Cached;
     }
-}
+    return Drv::CacheStatus::NotBuilt;
+};
+} // namespace
 
 /* The fields of a derivation that are printed in json form */
 Drv::Drv(std::string &attrPath, nix::EvalState &state,
@@ -104,11 +111,11 @@ Drv::Drv(std::string &attrPath, nix::EvalState &state,
 
     if (args.meta) {
         nlohmann::json meta_;
-        for (auto &metaName : packageInfo.queryMetaNames()) {
+        for (const auto &metaName : packageInfo.queryMetaNames()) {
             nix::NixStringContext context;
             std::stringstream ss;
 
-            auto metaValue = packageInfo.queryMeta(metaName);
+            auto *metaValue = packageInfo.queryMeta(metaName);
             // Skip non-serialisable types
             // TODO: Fix serialisation of derivations to store paths
             if (metaValue == nullptr) {
@@ -133,7 +140,7 @@ Drv::Drv(std::string &attrPath, nix::EvalState &state,
     auto drv = localStore->readDerivation(packageInfo.requireDrvPath());
     for (const auto &[inputDrvPath, inputNode] : drv.inputDrvs.map) {
         std::set<std::string> inputDrvOutputs;
-        for (auto &outputName : inputNode.value) {
+        for (const auto &outputName : inputNode.value) {
             inputDrvOutputs.insert(outputName);
         }
         inputDrvs[localStore->printStorePath(inputDrvPath)] = inputDrvOutputs;
@@ -144,7 +151,7 @@ Drv::Drv(std::string &attrPath, nix::EvalState &state,
 
 void to_json(nlohmann::json &json, const Drv &drv) {
     std::map<std::string, nlohmann::json> outputsJson;
-    for (auto &[name, optPath] : drv.outputs) {
+    for (const auto &[name, optPath] : drv.outputs) {
         outputsJson[name] =
             optPath ? nlohmann::json(*optPath) : nlohmann::json(nullptr);
     }
@@ -164,9 +171,16 @@ void to_json(nlohmann::json &json, const Drv &drv) {
         json["isCached"] = drv.cacheStatus == Drv::CacheStatus::Cached ||
                            drv.cacheStatus == Drv::CacheStatus::Local;
 
-        json["cacheStatus"] =
-            drv.cacheStatus == Drv::CacheStatus::Cached  ? "cached"
-            : drv.cacheStatus == Drv::CacheStatus::Local ? "local"
-                                                         : "notBuilt";
+        switch (drv.cacheStatus) {
+        case Drv::CacheStatus::Cached:
+            json["cacheStatus"] = "cached";
+            break;
+        case Drv::CacheStatus::Local:
+            json["cacheStatus"] = "local";
+            break;
+        default:
+            json["cacheStatus"] = "notBuilt";
+            break;
+        }
     }
 }

src/drv.hh

@@ -1,25 +1,23 @@
 #include <nix/get-drvs.hh>
 #include <nix/eval.hh>
-#include <nlohmann/json.hpp>
 #include <nlohmann/json_fwd.hpp>
 #include <cstdint>
 #include <string>
 #include <map>
 #include <set>
-#include <string>
 #include <optional>
 
 #include "eval-args.hh"
 
-class MyArgs;
-
 namespace nix {
 class EvalState;
 struct PackageInfo;
 } // namespace nix
 
 /* The fields of a derivation that are printed in json form */
 struct Drv {
+    Drv(std::string &attrPath, nix::EvalState &state,
+        nix::PackageInfo &packageInfo, MyArgs &args);
     std::string name;
     std::string system;
     std::string drvPath;
@@ -33,8 +31,5 @@ struct Drv {
     std::map<std::string, std::optional<std::string>> outputs;
     std::map<std::string, std::set<std::string>> inputDrvs;
     std::optional<nlohmann::json> meta;
-
-    Drv(std::string &attrPath, nix::EvalState &state,
-        nix::PackageInfo &packageInfo, MyArgs &args);
 };
 void to_json(nlohmann::json &json, const Drv &drv);