Enable ClangTidy alongside compilation in macOS (#1824)

Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>

Author: jviotti

CMakeLists.txt

@@ -28,9 +28,10 @@ option(SOURCEMETA_CORE_CONTRIB_GOOGLEBENCHMARK "Build the GoogleBenchmark librar
 
 include(Sourcemeta)
 
-# Don't force downstream consumers on it
+# Don't force downstream consumers on this
 if(PROJECT_IS_TOP_LEVEL)
   sourcemeta_enable_simd()
+  sourcemeta_clang_tidy_attempt_enable()
 endif()
 
 # TODO: Turn this into a re-usable utility CMake function
@@ -116,11 +117,13 @@ if(SOURCEMETA_CORE_DOCS)
 endif()
 
 if(PROJECT_IS_TOP_LEVEL)
+  # TODO: Try once more to enable this per target,
+  # so, we don't need to manually disable it here
+  sourcemeta_clang_tidy_attempt_disable()
   sourcemeta_target_clang_format(SOURCES
     src/*.h src/*.cc
     benchmark/*.h benchmark/*.cc
     test/*.h test/*.cc)
-  sourcemeta_target_clang_tidy(SOURCES src/*.cc)
 endif()
 
 # Testing

Makefile

@@ -25,9 +25,6 @@ compile: .always
 	$(CMAKE) --install ./build --prefix ./build/dist --config $(PRESET) --verbose \
 		--component sourcemeta_core_dev
 
-lint: .always
-	$(CMAKE) --build ./build --config $(PRESET) --target clang_tidy
-
 test: .always
 	$(CMAKE) -E env UBSAN_OPTIONS=print_stacktrace=1 \
 		$(CTEST) --test-dir ./build --build-config $(PRESET) \

cmake/Sourcemeta.cmake

@@ -10,11 +10,11 @@ include("${SOURCEMETA_UTILITIES_DIRECTORY}/commands/copy-file.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/library.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/executable.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/clang-format.cmake")
-include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/clang-tidy.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/shellcheck.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/doxygen.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/googletest.cmake")
 include("${SOURCEMETA_UTILITIES_DIRECTORY}/targets/googlebenchmark.cmake")
+include("${SOURCEMETA_UTILITIES_DIRECTORY}/clang-tidy.cmake")
 
 # To let downstream projects directly include this file
 if(NOT PROJECT_IS_TOP_LEVEL)

cmake/common/targets/clang-tidy.cmake renamed to cmake/common/clang-tidy.cmake

@@ -1,4 +1,4 @@
-function(sourcemeta_target_clang_tidy_attempt_install)
+function(sourcemeta_clang_tidy_attempt_install)
   cmake_parse_arguments(SOURCEMETA_TARGET_CLANG_TIDY_ATTEMPT_INSTALL "" "OUTPUT_DIRECTORY" "" ${ARGN})
   if(NOT SOURCEMETA_TARGET_CLANG_TIDY_ATTEMPT_INSTALL_OUTPUT_DIRECTORY)
     message(FATAL_ERROR "You must pass the output directory in the OUTPUT_DIRECTORY option")
@@ -76,9 +76,17 @@ function(sourcemeta_target_clang_tidy_attempt_install)
   message(STATUS "Installed `clang-tidy` pre-built binary to ${CLANG_TIDY_BINARY_OUTPUT}")
 endfunction()
 
-function(sourcemeta_target_clang_tidy)
-  cmake_parse_arguments(SOURCEMETA_TARGET_CLANG_TIDY "REQUIRED" "" "SOURCES" ${ARGN})
-  sourcemeta_target_clang_tidy_attempt_install(OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin")
+function(sourcemeta_clang_tidy_attempt_enable)
+  cmake_parse_arguments(SOURCEMETA_TARGET_CLANG_TIDY "REQUIRED" "" "" ${ARGN})
+
+  if(SOURCEMETA_COMPILER_LLVM)
+    message(STATUS "Enabling ClangTidy alongside compilation")
+  else()
+    message(STATUS "Ignoring ClangTidy setup on a compiler other than LLVM")
+    return()
+  endif()
+
+  sourcemeta_clang_tidy_attempt_install(OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/bin")
 
   if(SOURCEMETA_TARGET_CLANG_TIDY_REQUIRED)
     find_program(CLANG_TIDY_BIN NAMES clang-tidy NO_DEFAULT_PATH
@@ -94,37 +102,24 @@ function(sourcemeta_target_clang_tidy)
     endif()
   endif()
 
-
-  # This covers the empty list too
-  if(NOT SOURCEMETA_TARGET_CLANG_TIDY_SOURCES)
-    message(FATAL_ERROR "You must pass file globs to analyze in the SOURCES option")
-  endif()
-  file(GLOB_RECURSE SOURCEMETA_TARGET_CLANG_TIDY_FILES
-    ${SOURCEMETA_TARGET_CLANG_TIDY_SOURCES})
-
   set(CLANG_TIDY_CONFIG "${CMAKE_CURRENT_FUNCTION_LIST_DIR}/clang-tidy.config")
 
-  if(CMAKE_SYSTEM_NAME STREQUAL "MSYS")
-    # Because `clang-tidy` is typically a Windows `.exe`, transform the path accordingly
-    execute_process(COMMAND cygpath -w "${CLANG_TIDY_CONFIG}"
-      OUTPUT_VARIABLE CLANG_TIDY_CONFIG OUTPUT_STRIP_TRAILING_WHITESPACE)
-  endif()
-
-  if(CLANG_TIDY_BIN)
-    add_custom_target(clang_tidy
-      WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}"
-      VERBATIM
-      COMMAND "${CLANG_TIDY_BIN}" -p "${PROJECT_BINARY_DIR}"
-        --config-file "${CLANG_TIDY_CONFIG}"
-        ${SOURCEMETA_TARGET_CLANG_TIDY_FILES}
-        COMMENT "Analyzing sources using ClangTidy")
-  else()
-    add_custom_target(clang_tidy
-      WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}"
-      VERBATIM
-      COMMAND "${CMAKE_COMMAND}" -E echo "Could not locate ClangTidy"
-      COMMAND "${CMAKE_COMMAND}" -E false)
+  # TODO: Support other platforms too, like Linux
+  if(APPLE)
+    execute_process(COMMAND xcrun --show-sdk-path
+      OUTPUT_VARIABLE MACOSX_SDK_PATH OUTPUT_STRIP_TRAILING_WHITESPACE)
+    execute_process(COMMAND "${CMAKE_CXX_COMPILER}" -print-resource-dir
+      OUTPUT_VARIABLE MACOSX_RESOURCE_PATH OUTPUT_STRIP_TRAILING_WHITESPACE)
+    set(CMAKE_CXX_CLANG_TIDY
+      "${CLANG_TIDY_BIN};--config-file=${CLANG_TIDY_CONFIG};-header-filter=${PROJECT_SOURCE_DIR}/src/*"
+      "--extra-arg=-std=c++${CMAKE_CXX_STANDARD}"
+      "--extra-arg=-isysroot"
+      "--extra-arg=${MACOSX_SDK_PATH}"
+      "--extra-arg=-resource-dir=${MACOSX_RESOURCE_PATH}"
+      PARENT_SCOPE)
   endif()
+endfunction()
 
-  set_target_properties(clang_tidy PROPERTIES FOLDER "Linting")
+function(sourcemeta_clang_tidy_attempt_disable)
+  unset(CMAKE_CXX_CLANG_TIDY PARENT_SCOPE)
 endfunction()

cmake/common/clang-tidy.config

@@ -0,0 +1,19 @@
+---
+# See https://clang.llvm.org/extra/clang-tidy/index.html
+# First disable all default checks (with -*)
+Checks: '-*,
+          modernize-*'
+# TODO(bavulapati): iterate through the rules and enable them incrementally inorder to send smaller PRs
+          # bugprone-*,-bugprone-branch-clone,-bugprone-easily-swappable-parameters,-bugprone-empty-catch,
+          # clang-analyzer-*,
+          # clang-diagnostic-*,
+          # modernize-*,
+          # concurrency-*,
+          # cppcoreguidelines-*,-cppcoreguidelines-rvalue-reference-param-not-moved,
+          # performance-*,-performance-enum-size,
+          # portability-*,
+          # objc-*,
+          # misc-*,-misc-no-recursion,-misc-unused-parameters,-misc-const-correctness'
+WarningsAsErrors: '*'
+FormatStyle: none
+UseColor: true

cmake/common/targets/clang-tidy.config

@@ -56,31 +56,39 @@ template <typename Value> class JSONArray {
   /// Get a mutable end iterator on the array
   auto end() noexcept -> iterator { return this->data.end(); }
   /// Get a constant begin iterator on the array
-  auto begin() const noexcept -> const_iterator { return this->data.begin(); }
+  [[nodiscard]] auto begin() const noexcept -> const_iterator {
+    return this->data.begin();
+  }
   /// Get a constant end iterator on the array
-  auto end() const noexcept -> const_iterator { return this->data.end(); }
+  [[nodiscard]] auto end() const noexcept -> const_iterator {
+    return this->data.end();
+  }
   /// Get a constant begin iterator on the array
-  auto cbegin() const noexcept -> const_iterator { return this->data.cbegin(); }
+  [[nodiscard]] auto cbegin() const noexcept -> const_iterator {
+    return this->data.cbegin();
+  }
   /// Get a constant end iterator on the array
-  auto cend() const noexcept -> const_iterator { return this->data.cend(); }
+  [[nodiscard]] auto cend() const noexcept -> const_iterator {
+    return this->data.cend();
+  }
   /// Get a mutable reverse begin iterator on the array
   auto rbegin() noexcept -> reverse_iterator { return this->data.rbegin(); }
   /// Get a mutable reverse end iterator on the array
   auto rend() noexcept -> reverse_iterator { return this->data.rend(); }
   /// Get a constant reverse begin iterator on the array
-  auto rbegin() const noexcept -> const_reverse_iterator {
+  [[nodiscard]] auto rbegin() const noexcept -> const_reverse_iterator {
     return this->data.rbegin();
   }
   /// Get a constant reverse end iterator on the array
-  auto rend() const noexcept -> const_reverse_iterator {
+  [[nodiscard]] auto rend() const noexcept -> const_reverse_iterator {
     return this->data.rend();
   }
   /// Get a constant reverse begin iterator on the array
-  auto crbegin() const noexcept -> const_reverse_iterator {
+  [[nodiscard]] auto crbegin() const noexcept -> const_reverse_iterator {
     return this->data.crbegin();
   }
   /// Get a constant reverse end iterator on the array
-  auto crend() const noexcept -> const_reverse_iterator {
+  [[nodiscard]] auto crend() const noexcept -> const_reverse_iterator {
     return this->data.crend();
   }
 

src/core/json/include/sourcemeta/core/json_array.h

@@ -14,6 +14,7 @@ template <typename T> struct HashJSON {
     return value.fast_hash();
   }
 
+  [[nodiscard]]
   inline auto is_perfect(const hash_type) const noexcept -> bool {
     return false;
   }
@@ -45,6 +46,7 @@ template <typename T> struct PropertyHashJSON {
     }
   };
 
+  [[nodiscard]]
   inline auto perfect(const T &value, const std::size_t size) const noexcept
       -> hash_type {
     hash_type result;
@@ -134,6 +136,7 @@ template <typename T> struct PropertyHashJSON {
     }
   }
 
+  [[nodiscard]]
   inline auto is_perfect(const hash_type &hash) const noexcept -> bool {
     // If there is anything written past the first byte,
     // then it is a perfect hash

src/core/json/include/sourcemeta/core/json_hash.h

@@ -45,12 +45,21 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
     }
   }
 
-  auto begin() const noexcept -> const_iterator { return this->data.begin(); }
-  auto end() const noexcept -> const_iterator { return this->data.end(); }
-  auto cbegin() const noexcept -> const_iterator { return this->data.cbegin(); }
-  auto cend() const noexcept -> const_iterator { return this->data.cend(); }
+  [[nodiscard]] auto begin() const noexcept -> const_iterator {
+    return this->data.begin();
+  }
+  [[nodiscard]] auto end() const noexcept -> const_iterator {
+    return this->data.end();
+  }
+  [[nodiscard]] auto cbegin() const noexcept -> const_iterator {
+    return this->data.cbegin();
+  }
+  [[nodiscard]] auto cend() const noexcept -> const_iterator {
+    return this->data.cend();
+  }
 
-  inline auto hash(const key_type &key) const noexcept -> hash_type {
+  [[nodiscard]] inline auto hash(const key_type &key) const noexcept
+      -> hash_type {
     return this->hasher(key);
   }
 
@@ -134,7 +143,8 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
   }
 
   // As a performance optimisation if the hash is known
-  inline auto find(const key_type &key, const hash_type key_hash) const
+  [[nodiscard]] inline auto find(const key_type &key,
+                                 const hash_type key_hash) const
       -> const_iterator {
     assert(this->hash(key) == key_hash);
 
@@ -161,7 +171,8 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
     return this->cend();
   }
 
-  inline auto try_at(const key_type &key, const hash_type key_hash) const
+  [[nodiscard]] inline auto try_at(const key_type &key,
+                                   const hash_type key_hash) const
       -> const mapped_type * {
     assert(this->hash(key) == key_hash);
 
@@ -185,7 +196,8 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
   }
 
   // As a performance optimisation if the hash is known
-  auto contains(const key_type &key, const hash_type key_hash) const -> bool {
+  [[nodiscard]] auto contains(const key_type &key,
+                              const hash_type key_hash) const -> bool {
     assert(this->hash(key) == key_hash);
 
     // Move the perfect hash condition out of the loop for extra performance
@@ -208,7 +220,8 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
 
   // As a performance optimisation if the hash is known
 
-  inline auto at(const key_type &key, const hash_type key_hash) const
+  [[nodiscard]] inline auto at(const key_type &key,
+                               const hash_type key_hash) const
       -> const mapped_type & {
     assert(this->hash(key) == key_hash);
 
@@ -262,7 +275,8 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
 #endif
   }
 
-  inline auto at(const size_type index) const noexcept -> const Entry & {
+  [[nodiscard]] inline auto at(const size_type index) const noexcept
+      -> const Entry & {
     return this->data[index];
   }
 
@@ -317,9 +331,13 @@ template <typename Key, typename Value, typename Hash> class FlatMap {
     return this->erase(key, this->hash(key));
   }
 
-  inline auto size() const noexcept -> size_type { return this->data.size(); }
+  [[nodiscard]] inline auto size() const noexcept -> size_type {
+    return this->data.size();
+  }
 
-  inline auto empty() const noexcept -> bool { return this->data.empty(); }
+  [[nodiscard]] inline auto empty() const noexcept -> bool {
+    return this->data.empty();
+  }
 
   inline auto clear() noexcept -> void { this->data.clear(); }
 
@@ -418,44 +436,48 @@ template <typename Key, typename Value, typename Hash> class JSONObject {
   using const_pointer = typename Container::const_pointer;
   using const_iterator = typename Container::const_iterator;
 
-  inline auto begin() const noexcept -> const_iterator {
+  [[nodiscard]] inline auto begin() const noexcept -> const_iterator {
     return this->data.begin();
   }
   /// Get a constant end iterator on the object
-  inline auto end() const noexcept -> const_iterator {
+  [[nodiscard]] inline auto end() const noexcept -> const_iterator {
     return this->data.end();
   }
   /// Get a constant begin iterator on the object
-  inline auto cbegin() const noexcept -> const_iterator {
+  [[nodiscard]] inline auto cbegin() const noexcept -> const_iterator {
     return this->data.cbegin();
   }
   /// Get a constant end iterator on the object
-  inline auto cend() const noexcept -> const_iterator {
+  [[nodiscard]] inline auto cend() const noexcept -> const_iterator {
     return this->data.cend();
   }
 
   /// Attempt to find an entry by key
-  inline auto find(const Key &key) const -> const_iterator {
+  [[nodiscard]] inline auto find(const Key &key) const -> const_iterator {
     return this->data.find(key, this->data.hash(key));
   }
 
   /// Check if an entry with the given key exists
-  inline auto defines(const Key &key,
-                      const typename Container::hash_type hash) const -> bool {
+  [[nodiscard]] inline auto
+  defines(const Key &key, const typename Container::hash_type hash) const
+      -> bool {
     return this->data.contains(key, hash);
   }
 
   /// Check the size of the object
-  inline auto size() const -> std::size_t { return this->data.size(); }
+  [[nodiscard]] inline auto size() const -> std::size_t {
+    return this->data.size();
+  }
 
   /// Access an object entry by its underlying positional index
-  inline auto at(const size_type index) const noexcept -> const
+  [[nodiscard]] inline auto at(const size_type index) const noexcept -> const
       typename Container::Entry & {
     return this->data.at(index);
   }
 
   // Hash an object property
-  inline auto hash(const Key &property) const -> typename Container::hash_type {
+  [[nodiscard]] inline auto hash(const Key &property) const ->
+      typename Container::hash_type {
     return this->data.hasher(property);
   }