diff --git a/.clang-tidy b/.clang-tidy index d509f2cc..78de7315 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,155 +1,18 @@ --- -# Enable ALL the things! Except not really -# misc-non-private-member-variables-in-classes: the options don't do anything -# modernize-use-nodiscard: too aggressive, attribute is situationally useful -Checks: "*,\ - -google-readability-todo,\ - -altera-*,\ - -fuchsia-*,\ - fuchsia-multiple-inheritance,\ - -llvm-header-guard,\ - -llvm-include-order,\ - -llvmlibc-*,\ - -modernize-use-nodiscard,\ - -misc-non-private-member-variables-in-classes" +# Keep the initial CI policy focused on diagnostics that are actionable and +# complementary to cppcheck, sanitizers, and clang-format. +Checks: >- + clang-analyzer-*, + bugprone-*, + performance-*, + -bugprone-derived-method-shadowing-base-method, + -bugprone-easily-swappable-parameters, + -bugprone-macro-parentheses, + -bugprone-throwing-static-initialization, + -performance-avoid-endl, + -performance-enum-size, + -performance-unnecessary-value-param WarningsAsErrors: '' -CheckOptions: - - key: 'bugprone-argument-comment.StrictMode' - value: 'true' -# Prefer using enum classes with 2 values for parameters instead of bools - - key: 'bugprone-argument-comment.CommentBoolLiterals' - value: 'true' - - key: 'bugprone-misplaced-widening-cast.CheckImplicitCasts' - value: 'true' - - key: 'bugprone-sizeof-expression.WarnOnSizeOfIntegerExpression' - value: 'true' - - key: 'bugprone-suspicious-string-compare.WarnOnLogicalNotComparison' - value: 'true' - - key: 'readability-simplify-boolean-expr.ChainedConditionalReturn' - value: 'true' - - key: 'readability-simplify-boolean-expr.ChainedConditionalAssignment' - value: 'true' - - key: 'readability-uniqueptr-delete-release.PreferResetCall' - value: 'true' - - key: 'cppcoreguidelines-init-variables.MathHeader' - value: '' - - key: 'cppcoreguidelines-narrowing-conversions.PedanticMode' - value: 'true' - - key: 'readability-else-after-return.WarnOnUnfixable' - value: 'true' - - key: 'readability-else-after-return.WarnOnConditionVariables' - value: 'true' - - key: 'readability-inconsistent-declaration-parameter-name.Strict' - value: 'true' - - key: 'readability-qualified-auto.AddConstToQualified' - value: 'true' - - key: 'readability-redundant-access-specifiers.CheckFirstDeclaration' - value: 'true' -# These seem to be the most common identifier styles - - key: 'readability-identifier-naming.AbstractClassCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ClassCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ClassConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ClassMemberCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ClassMethodCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstantMemberCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstantParameterCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstantPointerParameterCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstexprFunctionCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstexprMethodCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ConstexprVariableCase' - value: 'lower_case' - - key: 'readability-identifier-naming.EnumCase' - value: 'lower_case' - - key: 'readability-identifier-naming.EnumConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.FunctionCase' - value: 'lower_case' - - key: 'readability-identifier-naming.GlobalConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.GlobalConstantPointerCase' - value: 'lower_case' - - key: 'readability-identifier-naming.GlobalFunctionCase' - value: 'lower_case' - - key: 'readability-identifier-naming.GlobalPointerCase' - value: 'lower_case' - - key: 'readability-identifier-naming.GlobalVariableCase' - value: 'lower_case' - - key: 'readability-identifier-naming.InlineNamespaceCase' - value: 'lower_case' - - key: 'readability-identifier-naming.LocalConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.LocalConstantPointerCase' - value: 'lower_case' - - key: 'readability-identifier-naming.LocalPointerCase' - value: 'lower_case' - - key: 'readability-identifier-naming.LocalVariableCase' - value: 'lower_case' - - key: 'readability-identifier-naming.MacroDefinitionCase' - value: 'UPPER_CASE' - - key: 'readability-identifier-naming.MemberCase' - value: 'lower_case' - - key: 'readability-identifier-naming.MethodCase' - value: 'lower_case' - - key: 'readability-identifier-naming.NamespaceCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ParameterCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ParameterPackCase' - value: 'lower_case' - - key: 'readability-identifier-naming.PointerParameterCase' - value: 'lower_case' - - key: 'readability-identifier-naming.PrivateMemberCase' - value: 'lower_case' - - key: 'readability-identifier-naming.PrivateMemberPrefix' - value: 'm_' - - key: 'readability-identifier-naming.PrivateMethodCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ProtectedMemberCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ProtectedMemberPrefix' - value: 'm_' - - key: 'readability-identifier-naming.ProtectedMethodCase' - value: 'lower_case' - - key: 'readability-identifier-naming.PublicMemberCase' - value: 'lower_case' - - key: 'readability-identifier-naming.PublicMethodCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ScopedEnumConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.StaticConstantCase' - value: 'lower_case' - - key: 'readability-identifier-naming.StaticVariableCase' - value: 'lower_case' - - key: 'readability-identifier-naming.StructCase' - value: 'lower_case' - - key: 'readability-identifier-naming.TemplateParameterCase' - value: 'CamelCase' - - key: 'readability-identifier-naming.TemplateTemplateParameterCase' - value: 'CamelCase' - - key: 'readability-identifier-naming.TypeAliasCase' - value: 'lower_case' - - key: 'readability-identifier-naming.TypedefCase' - value: 'lower_case' - - key: 'readability-identifier-naming.TypeTemplateParameterCase' - value: 'CamelCase' - - key: 'readability-identifier-naming.UnionCase' - value: 'lower_case' - - key: 'readability-identifier-naming.ValueTemplateParameterCase' - value: 'CamelCase' - - key: 'readability-identifier-naming.VariableCase' - value: 'lower_case' - - key: 'readability-identifier-naming.VirtualMethodCase' - value: 'lower_case' +HeaderFilterRegex: '^(.*[/\\])?src/' +FormatStyle: file ... diff --git a/.github/workflows/clang-tidy.yml b/.github/workflows/clang-tidy.yml new file mode 100644 index 00000000..ceaf793a --- /dev/null +++ b/.github/workflows/clang-tidy.yml @@ -0,0 +1,49 @@ +name: Clang-Tidy + +permissions: + contents: read + +on: + push: + branches: + - main + paths: + - 'src/**' + - '.clang-tidy' + - 'CMakeLists.txt' + - 'CMakePresets.json' + - 'cmake/**' + pull_request: + paths: + - 'src/**' + - '.clang-tidy' + - 'CMakeLists.txt' + - 'CMakePresets.json' + - 'cmake/**' + workflow_dispatch: + +jobs: + clang-tidy: + name: Run clang-tidy static analysis + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Install dependencies + run: | + sudo apt-get update + sudo apt-get install -y clang-tidy libhdf5-dev + git clone https://github.com/catchorg/Catch2.git + cd Catch2 + git checkout "v3.5.3" + cmake -Bbuild -H. -DBUILD_TESTING=OFF + sudo cmake --build build/ --target install + + - name: Configure compilation database + run: cmake --preset=ci-ubuntu -DCMAKE_EXPORT_COMPILE_COMMANDS=ON + + - name: Run clang-tidy + continue-on-error: true + run: cmake --build build --target tidy-check diff --git a/CHANGELOG.md b/CHANGELOG.md index 97e73860..88ba1e62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Added tutorial on using the ROS3 and remfile drivers to read NWB files in S3 (`docs/pages/userdocs/reads3.dox`) (@oruebel, [#308](https://github.com/NeurodataWithoutBorders/aqnwb/pull/308); @bendichter, [#309](https://github.com/NeurodataWithoutBorders/aqnwb/pull/309)) * Added `ElectricalSeries::writeAllChannels` method and `IO::writeElectricalSeriesData` overload to simplify zero-copy interleaved multichannel writes. (@copilot, @oruebel, [#293](https://github.com/NeurodataWithoutBorders/aqnwb/pull/293)) * Added `ElectricalSeries::channelsAtSameSampleOffset` method to check if all channels are at the same sample offset, which is a requirement for using `writeAllChannels`. (@copilot, @oruebel, [#293](https://github.com/NeurodataWithoutBorders/aqnwb/pull/293)) +* Added an advisory clang-tidy workflow and `tidy-check` CMake target using compilation databases to analyze focused correctness and performance checks on production source changes. ([#322](https://github.com/NeurodataWithoutBorders/aqnwb/pull/322)) * Added new `BaseIO::findObject` and `RegisteredType::findOwnedObject` methods to simplify searching for objects by name. Added `HDF5IO::findObject` override method to optimize the search for HDF5 objects. (@oruebel, [#308](https://github.com/NeurodataWithoutBorders/aqnwb/pull/308)) ### Changed diff --git a/CMakePresets.json b/CMakePresets.json index 02e28fd1..1c2b519a 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -36,13 +36,6 @@ "CMAKE_CXX_CPPCHECK": "cppcheck;--inline-suppr" } }, - { - "name": "clang-tidy", - "hidden": true, - "cacheVariables": { - "CMAKE_CXX_CLANG_TIDY": "clang-tidy;--header-filter=^${sourceDir}/" - } - }, { "name": "ci-std", "description": "This preset makes sure the project actually builds with at least the specified standard", diff --git a/cmake/clang-tidy-targets.cmake b/cmake/clang-tidy-targets.cmake new file mode 100644 index 00000000..8a07cb71 --- /dev/null +++ b/cmake/clang-tidy-targets.cmake @@ -0,0 +1,25 @@ +find_program( + default_clang_tidy_command + NAMES clang-tidy + HINTS + /opt/homebrew/opt/llvm/bin + /usr/local/opt/llvm/bin +) + +set( + CLANG_TIDY_COMMAND + "${default_clang_tidy_command}" + CACHE FILEPATH + "Static analyzer to use" +) + +add_custom_target( + tidy-check + COMMAND "${CMAKE_COMMAND}" + -D "CLANG_TIDY_COMMAND=${CLANG_TIDY_COMMAND}" + -D "COMPILE_COMMANDS_DIR=${PROJECT_BINARY_DIR}" + -P "${PROJECT_SOURCE_DIR}/cmake/clang-tidy.cmake" + WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}" + COMMENT "Running clang-tidy static analysis" + VERBATIM +) diff --git a/cmake/clang-tidy.cmake b/cmake/clang-tidy.cmake new file mode 100644 index 00000000..e8fa1040 --- /dev/null +++ b/cmake/clang-tidy.cmake @@ -0,0 +1,65 @@ +cmake_minimum_required(VERSION 3.15) + +macro(default name) + if(NOT DEFINED "${name}") + set("${name}" "${ARGN}") + endif() +endmacro() + +default(CLANG_TIDY_COMMAND clang-tidy) +default(COMPILE_COMMANDS_DIR "${CMAKE_BINARY_DIR}") +default(SOURCE_DIRS "src") + +if(NOT EXISTS "${COMPILE_COMMANDS_DIR}/compile_commands.json") + message(FATAL_ERROR + "clang-tidy requires a compilation database. Reconfigure with " + "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON." + ) +endif() + +get_filename_component(PROJECT_SOURCE_DIR "${CMAKE_CURRENT_LIST_DIR}/.." ABSOLUTE) + +set(source_files "") +foreach(dir IN LISTS SOURCE_DIRS) + file(GLOB_RECURSE dir_source_files "${PROJECT_SOURCE_DIR}/${dir}/*.cpp") + list(APPEND source_files ${dir_source_files}) +endforeach() + +if(NOT source_files) + message(FATAL_ERROR "No C++ source files found for clang-tidy analysis.") +endif() + +set(clang_tidy_args + -p "${COMPILE_COMMANDS_DIR}" + --warnings-as-errors=* +) + +if(APPLE) + execute_process( + COMMAND xcrun --show-sdk-path + OUTPUT_VARIABLE sdk_path + OUTPUT_STRIP_TRAILING_WHITESPACE + RESULT_VARIABLE sdk_result + ) + if(NOT sdk_result EQUAL "0") + message(FATAL_ERROR "Unable to determine the macOS SDK path for clang-tidy.") + endif() + list(APPEND clang_tidy_args + "--extra-arg=-isysroot${sdk_path}" + "--extra-arg=-isystem${sdk_path}/usr/include/c++/v1" + ) +endif() + +execute_process( + COMMAND "${CLANG_TIDY_COMMAND}" ${clang_tidy_args} ${source_files} + WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}" + RESULT_VARIABLE result + OUTPUT_VARIABLE output + ERROR_VARIABLE error_output +) + +if(NOT result EQUAL "0") + message(FATAL_ERROR + "clang-tidy failed with result '${result}':\n${output}${error_output}" + ) +endif() diff --git a/cmake/dev-mode.cmake b/cmake/dev-mode.cmake index f42da336..d72216f6 100644 --- a/cmake/dev-mode.cmake +++ b/cmake/dev-mode.cmake @@ -18,5 +18,6 @@ endif() include(cmake/lint-targets.cmake) include(cmake/spell-targets.cmake) include(cmake/cppcheck-targets.cmake) +include(cmake/clang-tidy-targets.cmake) add_folders(Project) diff --git a/docs/pages/devdocs/testing.dox b/docs/pages/devdocs/testing.dox index 6ae1cbb9..177b5227 100644 --- a/docs/pages/devdocs/testing.dox +++ b/docs/pages/devdocs/testing.dox @@ -109,4 +109,26 @@ * [GitHub Security](https://github.com/NeurodataWithoutBorders/aqnwb/security/code-scanning) * tab, where cppcheck results can be browsed and tracked over time. * + * \section testing_clang_tidy Static Analysis (clang-tidy) + * + * AqNWB uses ``clang-tidy`` as an advisory, compiler-aware static analysis check. + * Its focused initial policy covers Clang static-analyzer, ``bugprone``, and + * ``performance`` checks for production code. Unlike cppcheck, clang-tidy uses + * the CMake compilation database, so it analyzes the compiler flags, include + * paths, and definitions used by the project. + * + * To run it locally, first configure a build with an exported compilation database, + * then invoke the ``tidy-check`` target: + * + * \code{.sh} + * cmake --preset=dev -DCMAKE_EXPORT_COMPILE_COMMANDS=ON + * cmake --build --preset=dev --target=tidy-check + * \endcode + * + * The ``clang-tidy`` GitHub Actions workflow defined in + * [.github/workflows/clang-tidy.yml](https://github.com/NeurodataWithoutBorders/aqnwb/blob/main/.github/workflows/clang-tidy.yml) + * runs on changes to production source or analysis configuration. It invokes the + * same ``tidy-check`` target and analyzes all production translation units. + * Findings are advisory while initial CI coverage is established. + * */ diff --git a/src/io/hdf5/HDF5IO.cpp b/src/io/hdf5/HDF5IO.cpp index bc2930ba..a0e6440f 100644 --- a/src/io/hdf5/HDF5IO.cpp +++ b/src/io/hdf5/HDF5IO.cpp @@ -36,6 +36,7 @@ HDF5IO::HDF5IO(const std::string& fileName) { } +// NOLINTNEXTLINE(bugprone-exception-escape): teardown catches all close errors. HDF5IO::~HDF5IO() { try { @@ -460,7 +461,10 @@ std::vector HDF5IO::readStringDataHelper( if (strType.isVariableStr()) { // Handle variable-length strings std::vector buffer(numElements, nullptr); - dataset->read(buffer.data(), strType, memspace, dataspace); + dataset->read(reinterpret_cast(buffer.data()), + strType, + memspace, + dataspace); // Convert char* to std::string and free allocated memory for (size_t i = 0; i < numElements; ++i) { @@ -498,7 +502,7 @@ std::vector HDF5IO::readStringDataHelper( if (strType.isVariableStr()) { // Handle variable-length strings std::vector buffer(numElements, nullptr); - attribute->read(strType, buffer.data()); + attribute->read(strType, reinterpret_cast(buffer.data())); // Convert char* to std::string and free allocated memory for (size_t i = 0; i < numElements; ++i) { @@ -645,7 +649,7 @@ AQNWB::IO::DataBlockGeneric HDF5IO::readAttribute( // Handle variable-length strings std::vector stringData; std::vector buffer(numElements); - attribute.read(dataType, buffer.data()); + attribute.read(dataType, reinterpret_cast(buffer.data())); for (size_t i = 0; i < numElements; ++i) { stringData.emplace_back(buffer[i]); @@ -1010,7 +1014,7 @@ Status HDF5IO::createAttribute(const std::string& data, // Write the scalar string data const char* dataPtr = data.c_str(); - attr.write(nativeType, &dataPtr); + attr.write(nativeType, reinterpret_cast(&dataPtr)); } catch (const GroupIException& error) { error.printErrorStack(); @@ -1081,7 +1085,7 @@ Status HDF5IO::createAttribute(const std::vector& data, data.end(), dataPtrs.begin(), [](const std::string& str) { return str.c_str(); }); - attr.write(nativeType, dataPtrs.data()); + attr.write(nativeType, reinterpret_cast(dataPtrs.data())); } catch (const GroupIException& error) { error.printErrorStack(); @@ -1731,6 +1735,8 @@ H5::DataType HDF5IO::getNativeType(IO::BaseDataType type) AQNWB::IO::BaseDataType HDF5IO::getBaseDataType(const H5::DataType& nativeType) { + // NOLINTBEGIN(bugprone-branch-clone): distinct HDF5 native types map to + // distinct AqNWB types. if (nativeType == H5::PredType::NATIVE_INT8) { return IO::BaseDataType(IO::BaseDataType::Type::T_I8); } else if (nativeType == H5::PredType::NATIVE_INT16) { @@ -1770,6 +1776,7 @@ AQNWB::IO::BaseDataType HDF5IO::getBaseDataType(const H5::DataType& nativeType) // Default case: return a 32-bit integer type return IO::BaseDataType(IO::BaseDataType::Type::T_I32); } + // NOLINTEND(bugprone-branch-clone) } H5::DataType HDF5IO::getH5Type(IO::BaseDataType type) diff --git a/src/io/hdf5/HDF5RecordingData.cpp b/src/io/hdf5/HDF5RecordingData.cpp index 82570ed7..05d839ab 100644 --- a/src/io/hdf5/HDF5RecordingData.cpp +++ b/src/io/hdf5/HDF5RecordingData.cpp @@ -120,7 +120,10 @@ Status HDF5RecordingData::writeDataBlock(const SizeArray& dataShape, cstrBuffer[i] = data[i].c_str(); } // Write the data - m_dataset->write(cstrBuffer.data(), nativeType, mSpace, fSpace); + m_dataset->write(reinterpret_cast(cstrBuffer.data()), + nativeType, + mSpace, + fSpace); } else if (type.type == BaseDataType::Type::T_STR) { // Handle fixed-length strings std::vector buffer(data.size() * type.typeSize, '\0');