Add clang-tidy support

Add a CMake option `ENABLE_CLANG_TIDY` to enable `clang-tidy` for the
project.

This is enabled on Linux only in the Github runners since it is the only
image that has `clang-tidy` installed by default.

I have disabled any check that didn't seem to improve the code.  The
only code that was exempt and should be fixed is the recursive function
calls in `trimutil.cpp` triggered by `misc-no-recursion`.  I think
fixing them should be done separately to avoid an overly complex commit.

Author: elliotgoodrich

.clang-tidy

@@ -0,0 +1,37 @@
+---
+Checks: "bugprone-*,
+         misc-*,
+         -misc-non-private-member-variables-in-classes,
+         modernize-*,
+         -modernize-use-default-member-init,
+         -modernize-use-nodiscard,
+         -modernize-use-trailing-return-type,
+         performance-*,
+         readability-*,
+         -readability-convert-member-functions-to-static,
+         -readability-else-after-return,
+         -readability-implicit-bool-conversion,
+         -readability-magic-numbers,
+         -readability-named-parameter,
+         -readability-redundant-member-init,
+         -readability-qualified-auto,
+"
+WarningsAsErrors: "bugprone-*,
+                   misc-*,
+                   -misc-non-private-member-variables-in-classes,
+                   modernize-*,
+                   -modernize-use-default-member-init,
+                   -modernize-use-nodiscard,
+                   -modernize-use-trailing-return-type,
+                   performance-*,
+                   readability-*,
+                   -readability-convert-member-functions-to-static,
+                   -readability-else-after-return,
+                   -readability-implicit-bool-conversion,
+                   -readability-magic-numbers,
+                   -readability-named-parameter,
+                   -readability-redundant-member-init,
+                   -readability-qualified-auto,
+"
+HeaderFilterRegex: 'thirdparty/'
+FormatStyle: none

.github/workflows/ci.yaml

@@ -31,6 +31,7 @@ jobs:
         -DCMAKE_C_COMPILER=${{ matrix.environment.c }}
         ${{ format('-D{0}={1}', matrix.environment.name == 'Windows' && 'CMAKE_CONFIGURATION_TYPES' || 'CMAKE_BUILD_TYPE', matrix.build_type) }}
         ${{ matrix.environment.name != 'Windows' && '-G Ninja' || ''}}
+        ${{ matrix.environment.name == 'Linux' && '-DENABLE_CLANG_TIDY=ON' || ''}}
     - uses: elliotgoodrich/trimja-action@v1
       # Skip Windows (which doesn't yet use Ninja) and releases
       if: matrix.environment.name != 'Windows' && !startsWith(github.ref, 'refs/tags/')

CMakeLists.txt

@@ -6,6 +6,19 @@ set(CMAKE_CXX_STANDARD 20)
 set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_COMPILE_WARNING_AS_ERROR ON)
 
+# Add an option to enable or disable clang-tidy
+option(ENABLE_CLANG_TIDY "Enable clang-tidy static analysis" OFF)
+
+if(ENABLE_CLANG_TIDY)
+    find_program(CLANG_TIDY_EXE NAMES "clang-tidy")
+    if(CLANG_TIDY_EXE)
+        set(CMAKE_CXX_CLANG_TIDY "${CLANG_TIDY_EXE}")
+    else()
+        message(WARNING "clang-tidy not found. Static analysis will be skipped.")
+    endif()
+endif()
+
+
 ###############################################################################
 # trimja                                                                      #
 ###############################################################################
@@ -34,6 +47,13 @@ add_executable(
     $<$<BOOL:${WIN32}>:thirdparty/ninja/getopt.c>
 )
 
+set_source_files_properties(
+    thirdparty/ninja/lexer.cc
+    thirdparty/ninja/util.cc
+    thirdparty/ninja/getopt.cc
+    PROPERTIES SKIP_LINTING ON
+)
+
 target_compile_definitions(trimja PRIVATE TRIMJA_VERSION="${CMAKE_PROJECT_VERSION}")
 target_include_directories(trimja PRIVATE src)
 target_include_directories(trimja SYSTEM PRIVATE thirdparty)

src/basicscope.cpp

@@ -26,15 +26,15 @@ namespace trimja {
 
 BasicScope::BasicScope() = default;
 
-BasicScope::BasicScope(BasicScope&&) = default;
+BasicScope::BasicScope(BasicScope&&) noexcept = default;
 
 BasicScope::BasicScope(const BasicScope& other) {
   for (const auto& [name, value] : other.m_variables) {
     m_variables.emplace(name.view(), value);
   }
 }
 
-BasicScope& BasicScope::operator=(BasicScope&&) = default;
+BasicScope& BasicScope::operator=(BasicScope&&) noexcept = default;
 
 BasicScope& BasicScope::operator=(const BasicScope& rhs) {
   BasicScope tmp{rhs};
@@ -64,6 +64,5 @@ bool BasicScope::appendValue(std::string& output, std::string_view name) const {
     output += it->second;
     return true;
   }
-}
 
 }  // namespace trimja

src/basicscope.h

@@ -52,7 +52,7 @@ class BasicScope {
    * @brief Move constructor.
    * @param other The BasicScope to move from.
    */
-  BasicScope(BasicScope&& other);
+  BasicScope(BasicScope&& other) noexcept;
 
   /**
    * @brief Copy constructor.
@@ -65,7 +65,7 @@ class BasicScope {
    * @param rhs The BasicScope to move assign from.
    * @return A reference to the assigned BasicScope.
    */
-  BasicScope& operator=(BasicScope&& rhs);
+  BasicScope& operator=(BasicScope&& rhs) noexcept;
 
   /**
    * @brief Copy assignment operator.

src/cpuprofiler.cpp

@@ -46,7 +46,7 @@ Timer::~Timer() {
   stop();
 }
 
-void Timer::stop() {
+void Timer::stop() {  // NOLINT(readability-make-member-function-const)
   if (m_output) {
     *m_output = std::chrono::steady_clock::now() - m_start;
   }

src/depsreader.cpp

@@ -36,7 +36,7 @@ namespace {
 const std::size_t NINJA_MAX_RECORD_SIZE = 0b11'1111'1111'1111'1111;
 
 template <typename TYPE>
-TYPE readBinary(std::istream& in) {
+TYPE readFromStream(std::istream& in) {
   TYPE value;
   in.read(reinterpret_cast<char*>(&value), sizeof(value));
   return value;
@@ -86,7 +86,7 @@ DepsReader::DepsReader(const std::filesystem::path& ninja_deps)
     throw std::runtime_error("Unable to find ninjadeps signature");
   }
 
-  const std::int32_t header = readBinary<std::int32_t>(m_deps);
+  const auto header = readFromStream<std::int32_t>(m_deps);
   if (header != 4) {
     throw std::runtime_error("Only version 4 supported of ninjadeps");
   }
@@ -97,7 +97,7 @@ bool DepsReader::read(std::variant<PathRecordView, DepsRecordView>* output) {
     if (m_deps.peek() == EOF) {
       return false;
     }
-    const std::uint32_t rawRecordSize = readBinary<std::uint32_t>(m_deps);
+    const auto rawRecordSize = readFromStream<std::uint32_t>(m_deps);
     const std::uint32_t recordSize = rawRecordSize & 0x7FFFFFFF;
     if (recordSize > NINJA_MAX_RECORD_SIZE) {
       throw std::runtime_error("Record exceeding the maximum size found");
@@ -110,19 +110,18 @@ bool DepsReader::read(std::variant<PathRecordView, DepsRecordView>* output) {
       const std::size_t padding =
           (end[-1] == '\0') + (end[-2] == '\0') + (end[-3] == '\0');
       m_storage.erase(m_storage.size() - padding);
-      const std::uint32_t checksum = readBinary<std::uint32_t>(m_deps);
+      const auto checksum = readFromStream<std::uint32_t>(m_deps);
       const std::int32_t id = ~checksum;
       *output = PathRecordView{id, m_storage};
     } else {
-      const std::int32_t outIndex = readBinary<std::int32_t>(m_deps);
-      const ninja_clock::time_point mtime =
-          readBinary<ninja_clock::time_point>(m_deps);
+      const auto outIndex = readFromStream<std::int32_t>(m_deps);
+      const auto mtime = readFromStream<ninja_clock::time_point>(m_deps);
       const std::uint32_t numDependencies =
           (recordSize - sizeof(std::int32_t) - 2 * sizeof(std::uint32_t)) /
           sizeof(std::int32_t);
       m_depsStorage.resize(numDependencies);
       std::generate_n(m_depsStorage.begin(), numDependencies,
-                      [&] { return readBinary<std::int32_t>(m_deps); });
+                      [&] { return readFromStream<std::int32_t>(m_deps); });
       *output = DepsRecordView{outIndex, mtime, m_depsStorage};
     }
   } catch (const std::ios_base::failure& e) {

src/edgescope.cpp

@@ -24,8 +24,7 @@
 
 #include <ninja/util.h>
 
-namespace trimja {
-namespace detail {
+namespace trimja::detail {
 
 void appendPaths(std::string& output,
                  std::span<const std::string> paths,
@@ -44,6 +43,4 @@ void appendPaths(std::string& output,
   }
 }
 
-}  // namespace detail
-
-}  // namespace trimja
+}  // namespace trimja::detail

src/evalstring.cpp

@@ -58,7 +58,7 @@ EvalString::Offset setLeadingBit(EvalString::Offset in) {
 }
 
 bool hasLeadingBit(EvalString::Offset in) {
-  return in >> (std::numeric_limits<EvalString::Offset>::digits - 1u);
+  return in >> (std::numeric_limits<EvalString::Offset>::digits - 1U);
 }
 
 }  // namespace

src/graph.cpp

@@ -35,14 +35,14 @@ std::size_t Graph::PathHash::operator()(const fixed_string& v) const {
 std::size_t Graph::PathHash::operator()(std::string_view v) const {
   // FNV-1a hash algorithm but on Windows we swap all backslashes with forward
   // slashes
-  std::size_t hash = 14695981039346656037ull;
+  std::size_t hash = 14695981039346656037ULL;
   for (const char ch : v) {
 #ifdef _WIN32
     hash ^= ch == '\\' ? '/' : ch;
 #else
     hash ^= ch;
 #endif
-    hash *= 1099511628211ull;
+    hash *= 1099511628211ULL;
   }
   return hash;
 }
@@ -132,7 +132,7 @@ std::size_t Graph::addDefault() {
   const std::size_t nextIndex = m_inputToOutput.size();
   m_inputToOutput.emplace_back();
   m_outputToInput.emplace_back();
-  m_path.push_back("default");
+  m_path.emplace_back("default");
   m_defaultIndex = nextIndex;
   return nextIndex;
 }

src/murmur_hash.cpp

@@ -34,7 +34,7 @@ std::uint64_t murmur_hash::hash(const void* key, std::size_t length) {
   // did too), to use a hard-coded seed that is used within `ninja`, and to
   // generally be brought up to modern C++ standards.
 
-  const unsigned char* data = static_cast<const unsigned char*>(key);
+  const auto* data = static_cast<const unsigned char*>(key);
 
   const std::uint64_t m = 0xc6a4'a793'5bd1'e995;
   const int r = 47;

src/trimja.m.cpp

@@ -96,7 +96,8 @@ branch, note the lone '-' argument to specify we are reading from stdin,
 
 For more information visit the homepage https://github.com/elliotgoodrich/trimja)HELP";
 
-static const option g_longOptions[] = {
+// NOLINTNEXTLINE(modernize-avoid-c-arrays)
+const option g_longOptions[] = {
     // TODO: Remove `--expected` and replace with comparing files within CTest
     {"builddir", no_argument, nullptr, 'b'},
     {"explain", no_argument, nullptr, 'e'},
@@ -163,7 +164,7 @@ bool instrumentMemory = false;
   bool explain = false;
   bool builddir = false;
 
-  int ch;
+  int ch = -1;
   while ((ch = getopt_long(argc, argv, "a:f:ho:vw", g_longOptions, nullptr)) !=
          -1) {
     switch (ch) {

src/trimutil.cpp

@@ -663,7 +663,7 @@ void parseDepFile(const std::filesystem::path& ninjaDeps,
        DepsReader(ninjaDeps)) {
     switch (record.index()) {
       case 0: {
-        const PathRecordView& view = std::get<PathRecordView>(record);
+        const auto& view = std::get<PathRecordView>(record);
         if (static_cast<std::size_t>(view.index) >= paths.size()) {
           paths.resize(view.index + 1);
         }
@@ -672,7 +672,7 @@ void parseDepFile(const std::filesystem::path& ninjaDeps,
         break;
       }
       case 1: {
-        const DepsRecordView& view = std::get<DepsRecordView>(record);
+        const auto& view = std::get<DepsRecordView>(record);
         if (static_cast<std::size_t>(view.outIndex) >= deps.size()) {
           deps.resize(view.outIndex + 1);
         }
@@ -772,6 +772,7 @@ void parseLogFile(const std::filesystem::path& ninjaLog,
 // If `index` has not been seen (using `seen`) then call
 // `markIfChildrenAffected` for all inputs to `index` and then set
 // `isAffected[index]` if any child is affected. Return whether this
+// NOLINTNEXTLINE(misc-no-recursion)
 void markIfChildrenAffected(std::size_t index,
                             std::vector<bool>& seen,
                             std::vector<bool>& isAffected,
@@ -814,7 +815,8 @@ void markIfChildrenAffected(std::size_t index,
 
 // If `index` has not been seen (using `seen`) then call
 // `ifAffectedMarkAllChildren` for all outputs to `index` and then set
-// `isAffected[index]` if any child is affected. Return whether this
+// `isAffected[index]` if any child is affected.
+// NOLINTNEXTLINE(misc-no-recursion)
 void ifAffectedMarkAllChildren(std::size_t index,
                                std::vector<bool>& seen,
                                std::vector<bool>& isAffected,