integrating clang-tidy into openal-soft with cmake

[SeanTolstoyevski]

this changes introduces the integration of Clang-Tidy into the OpenAL-Soft project using CMake.
Clang-Tidy is a static code analysis tool that helps identify and fix potential issues in C++ code.

The changes in this pull request will analyze and apply clang-tidy fixes to the following directories and files:

• al
• alc
• common
• core
• utils/openal-info.c (if enabled)

requirements

• CMake 3.5 or higher see. CMAKE_EXPORT_COMPILE_COMMANDS. compatible for current project cmake version.
• Clang-Tidy (only required if analysis is executed)

usage

1. Configure OpenAL-Soft with CMake using the desired features and add the -DCMAKE_EXPORT_COMPILE_COMMANDS=ON flag to the CMake command line. And you can also set the appropriate clang-tidy program according to the platform, check out the note below.
   e.g. cmake -G "MinGW Makefiles" -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release ..
2. Run Clang-Tidy using the target clang-tidy-check.
   e.g. make clang-tidy-check

Note: LLVM toolset is usually installed as <tool_name>-<version>. The clang-tidy executable file may not be directly on your platform with the clang-tidy name/path. In this case, you can report clang-tidy available on the platform to cmake.
The default executable is clang-tidy.

• For Linux based OSs:

  • Method 1: Before configuring the project with cmake, the environment variable can be defined with export.
    E.g. export CLANG_TIDY_EXECUTABLE=clang-tidy-18
  • Method 2: It can be passed directly to cmake as a flag. .
    E.g. cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCLANG_TIDY_EXECUTABLE=clang-tidy-18 ..

• For Windows OS:

  • Method 1: Before configuring the project with cmake, environment variable can be defined with SET.
    E.g. SET CLANG_TIDY_EXECUTABLE=clang-tidy-18
  • Method 2 does not work on Windows.

importants

1. The fixes applied by clang-tidy are not guaranteed to be correct. Therefore, it is important to re-compile and test the project after running this target.
2. More checks over clang-tidy can be implemented, but this may prevent the project from compiling. A more comprehensive clang-tidy configuration with lint comments to ignore specific cases can be developed in the future.

some questions we need to answer

1. Should it be applied to external libraries/files embedded in the project? example. pffft.
2. What are the necessary / unnecessary checks?

I can make changes based on these two questions.

For the second question i took the basic checks and tested some of them to see if they were compatible with the project. Activating some of them (ex. using explicit in single-argument constructors) requires a lot of time because it requires too many changes. I eliminated these.

[kcat]

Should it be applied to external libraries/files embedded in the project? example. pffft.

For pffft, sure. It's already heavily modified, so more changes to fix other issues isn't a problem (so long as it doesn't adversely affect performance).

What are the necessary / unnecessary checks?

Currently this is what I have enabled with Qt Creator:

-*,bugprone-argument-comment,bugprone-assert-side-effect,bugprone-assignment-in-if-condition,
bugprone-bad-signal-to-kill-thread,bugprone-bool-pointer-implicit-conversion,
bugprone-copy-constructor-init,bugprone-dangling-handle,bugprone-dynamic-static-initializers,
bugprone-fold-init-type,bugprone-forward-declaration-namespace,
bugprone-forwarding-reference-overload,bugprone-implicit-widening-of-multiplication-result,
bugprone-inaccurate-erase,bugprone-incorrect-roundings,bugprone-infinite-loop,
bugprone-integer-division,bugprone-lambda-function-name,bugprone-macro-repeated-side-effects,
bugprone-misplaced-*,bugprone-move-forwarding-reference,bugprone-multiple-statement-macro,
bugprone-narrowing-conversions,bugprone-no-escape,bugprone-not-null-terminated-result,
bugprone-parent-virtual-call,bugprone-posix-return,bugprone-redundant-branch-condition,
bugprone-reserved-identifier,bugprone-shared-ptr-array-mismatch,bugprone-signal-handler,
bugprone-signed-char-misuse,bugprone-sizeof-*,bugprone-spuriously-wake-up-functions,
bugprone-standalone-empty,bugprone-string-*,bugprone-stringview-nullptr,bugprone-suspicious-*,
bugprone-swapped-arguments,bugprone-terminating-continue,bugprone-throw-keyword-missing,
bugprone-too-small-loop-variable,bugprone-undefined-memory-manipulation,
bugprone-undelegated-constructor,bugprone-unhandled-*,bugprone-unused-*,
bugprone-use-after-move,bugprone-virtual-near-miss,clang-*,
concurrency-thread-canceltype-asynchronous,cppcoreguidelines-avoid-c-arrays,
cppcoreguidelines-avoid-goto,cppcoreguidelines-avoid-reference-coroutine-parameters,
cppcoreguidelines-c-copy-assignment-signature,cppcoreguidelines-explicit-virtual-functions,
cppcoreguidelines-interfaces-global-init,cppcoreguidelines-narrowing-conversions,
cppcoreguidelines-no-malloc,cppcoreguidelines-owning-memory,
cppcoreguidelines-prefer-member-initializer,
cppcoreguidelines-pro-bounds-array-to-pointer-decay,
cppcoreguidelines-pro-bounds-pointer-arithmetic,cppcoreguidelines-pro-type-const-cast,
cppcoreguidelines-pro-type-cstyle-cast,cppcoreguidelines-pro-type-member-init,
cppcoreguidelines-pro-type-union-access,cppcoreguidelines-slicing,
cppcoreguidelines-virtual-class-destructor,modernize-avoid-*,modernize-concat-nested-namespaces,
modernize-deprecated-*,modernize-loop-convert,modernize-macro-to-enum,modernize-make-*,
modernize-pass-by-value,modernize-raw-string-literal,modernize-redundant-void-arg,
modernize-replace-*,modernize-return-braced-init-list,modernize-shrink-to-fit,
modernize-unary-static-assert,modernize-use-auto,modernize-use-bool-literals,
modernize-use-default-member-init,modernize-use-emplace,modernize-use-equals-*,
modernize-use-nodiscard,modernize-use-noexcept,modernize-use-nullptr,modernize-use-override,
modernize-use-transparent-functors,modernize-use-uncaught-exceptions,
modernize-use-using,portability-*,readability-const-return-type,readability-container-contains,
readability-container-size-empty,readability-convert-member-functions-to-static,
readability-delete-null-pointer,readability-duplicate-include,readability-else-after-return,
readability-inconsistent-declaration-parameter-name,readability-make-member-function-const,
readability-misleading-indentation,readability-misplaced-array-index,readability-redundant-*,
readability-simplify-subscript-expr,readability-static-definition-in-anonymous-namespace,
readability-string-compare,readability-uniqueptr-delete-release,readability-use-anyofallof

This has a few "false" reports from cppcoreguidelines-pro-bounds-pointer-arithmetic, dependent on implementation details for STL iterators. I'd rather not disable it since it's a more significant safety check.

[SeanTolstoyevski]

hi @kcat,

i added the checks you shared.

i tested the changes on ubuntu and Windows.

there seems to be no problem, but of course i think a review is necessary.

commit was a big :) .
i also made changes to cmake to be able to use clang-tidy on different platforms. this was something i forgot. if there are possible improvements in this, please let me know.

[SeanTolstoyevski]

Hi @kcat .

Sorry I've been MIA with this for a few months. It back from the dead :) .

• I've alphabetized and removed duplicate checks in .clang-tidy for a cleaner look.
• I've cleaned up some unnecessary lines in CMakeLists.txt.

I've created a patch that highlights the changes made by clang-tidy. We can review the checks based on this.
If I commit everything that clang-tidy does, it'll get really messy.

openal-soft-clang-tidy-changes.patch

[kcat]

Apologies for being rather non-responsive about this. It's been something I wanted to look at, but kept forgetting as I wanted to avoid doing anything too significant before the release, and I wasn't sure exactly what it was going to do to. Now that the release is out, though, I can take a more focused look at it.

I'm not terribly familiar with how clang-tidy itself functions, but I see it's passing the --fix and --fix-errors parameters, which says they apply suggested fixes. Does that directly change the source files in the source tree, or generate some kind of patch or modified copy? I'm a bit hesitant to have an automated check apply changes automatically to the sources across the whole codebase, as that seems like it could create problems with false-positives, misapply changes that may or may not get caught by the compiler, or do unrelated changes at the same time, where it's preferred to keep each commit focused on one thing when possible (there definitely are commits that don't do that, but it's something I strive for).

If it instead just outputs something with the fixes without directly modifying the sources, that should be fine. Or perhaps separate targets, one which does the checks and reports the suggested fixes that can be looked at, and another target that can apply those fixes if appropriate. I'm not really sure what the usual method of integrating clang-tidy into the build is for projects.

[SeanTolstoyevski]

Does that directly change the source files in the source tree

Yes, it makes changes directly to the source files for errors it can fix or similar things. 

You're right about incorrect warnings (although I don't see any I've experienced outside of include ordering for my projects and jobs projects). I think this is a risk that should be taken into account, especially in a project with many mathematical operations, as it can be difficult to detect these false positives.

If we prefer to avoid direct modifications to the source tree, we can use clang-tidy solely for feedback. By removing the --fix and --fix-error flags, clang-tidy will only highlight potential issues.

As the initial developer of the project, you've already modernized it significantly and clang-tidy approved with this approach. However it can serve as a valuable tool for future contributors, helping them ensure code compatibility with modern C++ standards or another things.

If it instead just outputs something with the fixes without directly modifying the sources

That's possible. We could consider redirecting clang-tidy's console output to a file.

[kcat]

That's possible. We could consider redirecting clang-tidy's console output to a file.

There seems to be an --export-fixes=<filename> command line option, though it says it outputs it to a YAML file, which might not be very human-readable (it says it can be used with clang-apply-replacements to apply the changes).

Are you familiar with any other projects that have clang-tidy integrated into their cmake script like this? If so, what do they normally do?

[SeanTolstoyevski]

There are several examples of projects, most do not use modern CMake. I don't know of a popular open source project, I'll look into it.

https://github.com/the-risk-taker/cppcheck-clang-tidy-and-cmake
https://github.com/johnthagen/clang-blueprint

If the project is quite large and complex, they may prefer to run it with Python scripts (example llvm project).

[kcat]

Removing the --fix and --fix-errors options should probably be good enough to start with.

[SeanTolstoyevski]

hi @kcat
I removed these flags for this target.