Introduce clang-tidy #920

pjungkamp · 2025-06-04T22:09:52Z

What is `clang-tidy`

clang-tidy is a linter developed as part of the clang-tools. This linter makes use of the clang C/C++ compiler front end to analyze and lint based on the actual C/C++ AST instead of the typical regex-based lints used by e.g. cppcheck.

This allows clang-tidy to automatically fix some of the errors it detects automatically. It is also nicely integrated with other clang tooling like the clangd language server.

Progress

Compile villas-node using clang and fix compiler warnings/errors
Run clang-tidy with a subset of checks enabled as a baseline check
- modernize-use-override: Enfore CppCoreGuideline C.128. Refactor and cleanups #907 (comment)
- misc-const-correctness: Add const to all variables that can be.
- clang-analyzer-*: Use clang static analyzer to find memory leaks, null dereferences, use-after-free and other bugs hidden in control flow or non-exception-safe code.
Detect and run clang-tidy automatically through cmake's CMAKE_CXX_CLANG_TIDY integration.
Enforce a subset of warnings as errors in pre-commit (and therefore the CI)

pjungkamp · 2025-06-04T22:41:09Z

I'd like to also enable readability-isolate-declaration. This is more of a style choice, less about correctness.

pjungkamp · 2025-06-04T22:47:15Z

common/lib/memory.cpp

The LinearAllocator triggered the clang-analyzer "dead-write" warning. I think this is definitely broken!

Okay, im fine with leaving it as is for now.. I am not aware of anybody using this code.

Maybe @n-eiling

I think the FPGA code is only using the HostRamAllocator. What is a dead-write warning?

A dead write is a write to non-volatile memory that is never read for the remainder of that memory's lifetime. This constructor modifies the function parameters after the member variables have been assigned by the member-initialization-list. This means that these modifications will just be discarded at the end of a function.

pjungkamp · 2025-06-04T22:50:54Z

@stv0g I added the NIX_LD and NIX_LD_LIBRARY_PATH environment variables to the devShells to allow the prebuilt pre-commit binaries to run on NixOS, which is missing a system-wide dynamic linker.

See https://github.com/nix-community/nix-ld?tab=readme-ov-file#usage

stv0g · 2025-06-05T05:13:58Z

Great work :-)

This PR is getting a bit too large for confidently merging and reviewing it.

Especially the clang-tidy commit (cab6e02) is quite big..

I would favour to merge this as is now, or splitting up the PR into smaller ones which can be merged immediatly.

n-eiling

In general this looks like it improves code quality quite a bit, espc. regarding override/virtual. However, I'm not convinced of adding const everywhere. Surely the compiler will do this optimization automatically. I doubt it adds much value during programming. I get the value for function parameters, especially if part of an interface, but for local variables I don't see the point.
I want us to focus on making contributions to VILLAS easier, and forcing people to use const everywhere may not be a move in this direction, or what do you think?

Also again: Please do not do style and logic changes in one PR, especially a PR of this size.

CMakeLists.txt

n-eiling · 2025-06-05T06:46:37Z

common/include/villas/kernel/vfio_device.hpp


  // libpci handle of the device
-  const kernel::devices::PciDevice *pci_device;
+  [[maybe_unused]] const kernel::devices::PciDevice *pci_device;


Is this really something we want to introduce? It adds clutter to the code. In this instance, we should check whether we actually need this (now or in the future) and rather remove it.
Anything touching the FPGA code should be tested by someone with access to the FPGA before we should approve changes.

This private member variable was set in the constructor, yet never read or used anywhere. I didn't know whether this was some kind of guard variable keeping a PCI connection alive or something like that.

I suspect it's a leftover from when vfio devices were always PCI devices. If it's really unused now, we can remove it.

n-eiling · 2025-06-05T06:57:08Z

common/lib/kernel/devices/pci_device.cpp

+    auto line = std::unique_ptr<char, decltype(line_destructor)>{
+        line_ptr, line_destructor};
+    if (ret == -1)
+      break;


I don't see the point of this. It's way less readable than the C code. If you absolutely must use C++, then why not use proper C++ file I/O? Also any change here needs to be tested by someone with access to an FPGA.

n-eiling · 2025-06-05T06:58:05Z

common/lib/kernel/kernel.cpp

-  FILE *f = fopen(PROCFS_PATH "/cmdline", "r");
+  auto file_destructor = [](FILE *file) { fclose(file); };
+  auto f = std::unique_ptr<FILE, decltype(file_destructor)>{
+      fopen(PROCFS_PATH "/cmdline", "r"), file_destructor};


Same here. I don't see the point of this change. Also needs FPGA testing.

clang-analyzer found a control flow path that lead to fclose not being called here. More specifically, it's missing an fclose before the return 0; // FOUND line. It also guards against leaks if an exception is thrown (I don't currently see any exception throwing code here, but I'd argue that using smart pointers for resource owning pointers is a better and less error-prone programming style.

Ok makes sense. Still you are mixing C and C++ code here, which I think is a bit ugly. Also the syntax is quite a bit more complex. I'm fine with your change, but I would prefer switching to a C++ API, or fixing the existing code.

n-eiling · 2025-06-05T06:58:13Z

common/lib/kernel/kernel.cpp

-  f = fopen(fn, "w+");
+  auto file_destructor = [](FILE *file) { fclose(file); };
+  auto f = std::unique_ptr<FILE, decltype(file_destructor)>{fopen(fn, "w+"),
+                                                            file_destructor};


n-eiling · 2025-06-05T07:31:15Z

src/villas-conf2json.cpp

-    FILE *f = fopen(argv[1], "r");
+    auto file_destructor = [](FILE *file) { fclose(file); };
+    auto f = std::unique_ptr<FILE, decltype(file_destructor)>{
+        fopen(argv[1], "r"), file_destructor};


I think this is less readable than the C code, and the better way would be to use C++ file I/O

tests/unit/format.cpp

n-eiling · 2025-06-05T07:33:22Z

tests/unit/format.cpp

 #endif

  rewind(stream);
+  (void)errno; // TODO: should we handle rewind errors?


Maybe make an issue

n-eiling · 2025-06-05T07:34:19Z

tests/unit/queue.cpp

                   p->many ? producer_consumer_many : producer_consumer, p);

-  sleep(0.2);
+  // sleep(0.2); TODO: Is this supposed to to something? Yield?


Dead code. Can we understand why this was here before just removing it?

n-eiling · 2025-06-05T07:34:56Z

tools/format-all.sh

-                -not \( -path "${TOP_DIR}/fpga/thirdparty/*" -o -path "${TOP_DIR}/build*/*" \) | \
-    xargs clang-format --verbose -i
+git ls-files -c -z -- "*.c" ".h" "*.hpp" "*.cpp" ":!:fpga/thirdparty" |\
+    xargs -0 clang-format --verbose -i


what are the implications of this change?

It does everything as before, except it only formats files known to git and works with paths that contain spaces and even newlines.

n-eiling · 2025-06-05T08:03:32Z

And any changes on what we expect from contributions should also be added here: https://villas.fein-aachen.org/docs/node/development/contributing Let's not let this get out of date again.

pjungkamp · 2025-06-05T08:32:48Z

I want to emphasize that this PR is not yet ready. I just wanted to track the progress through the GitHub PR checklist and discuss what lints to enable initially.

I want to split the large enable clang tidy commit by the lint that it fixed. I can also split out a base commit that just fixes the clang build.

I'll also add a related PR to the style guide documentation before this is ready to review.

stv0g · 2025-06-12T05:11:45Z

I want to emphasize that this PR is not yet ready.

Could we split out-some commits into small PRs please? There are multiple commits which deserve their own PR and we can review and merge those quicker.

Thanks :)

stv0g · 2025-10-30T11:06:01Z

I started to merge some unreleated changes of this PR via

pjungkamp · 2026-01-25T00:38:46Z

I've redone the whole clang-tidy cleanup from scratch and thus removed some of the fixes I had applied before. Especially the fixes for static analyzer findings were still up for discussion.

This PR will "only" fix the Clang build, add a clang-tidy file with the modernize-use-override lint as the baseline.

Other lints and the CI integration should be added in separate MRs.

pjungkamp · 2026-01-25T01:07:34Z

The test:python failure in the CI is unrelated to this MR.

pjungkamp · 2026-01-25T11:11:06Z

I fixed the pkg:nix:arx CI by switching from NixOS/bundlers to nix-community/nix-bundle.

https://github.com/NixOS/bundlers/blob/88ada423d9086c5e556da4243086f03d2d030f76/flake.nix#L52

This shouldn't affect behavior since NixOS/bundlers#bundlers.${system}.toArx uses a slightly out-of-date nix-community/nix-bundle under the hood, which caused the CI to fail.

pjungkamp · 2026-01-25T13:20:21Z

It seems like there's some bug with the CI artifact upload. Maybe it's out of artifact/container storage? Looks like the entire CI broke down.

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

…nt64_t Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

stv0g · 2026-01-26T09:12:28Z

I fixed the broken test:python and packaging:nix:arx CI jobs in master and rebased this PR onto it.

stv0g

Thanks :)

pjungkamp force-pushed the clang-tidy branch 4 times, most recently from 1003d2a to 3f9dfc6 Compare June 4, 2025 22:34

pjungkamp commented Jun 4, 2025

View reviewed changes

n-eiling requested changes Jun 5, 2025

View reviewed changes

stv0g mentioned this pull request Oct 30, 2025

FPGA cleanups #967

Merged

pjungkamp force-pushed the clang-tidy branch 2 times, most recently from d4ddbe6 to 3ff5b12 Compare January 25, 2026 00:38

pjungkamp marked this pull request as ready for review January 25, 2026 00:38

pjungkamp requested review from n-eiling and stv0g January 25, 2026 00:52

pjungkamp force-pushed the clang-tidy branch from 3ff5b12 to 1992d52 Compare January 25, 2026 00:57

stv0g mentioned this pull request Jan 26, 2026

fix(ci/arx): Use nix-community/nix-bundle instead of NixOS/bundlers #994

Merged

pjungkamp added 5 commits January 26, 2026 10:10

Introduce .clang-tidy

5135288

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

nix: Add clang based package and devShell

c422b4c

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

Make villas::utils::PopenStream operator>> a friend

61832cc

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(vfio_device): Remove unused pci_device pointer

f53bc12

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(vfio_device): Make IrqVectorInfo initialization more explicit

4eba32c

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

pjungkamp added 21 commits January 26, 2026 10:11

fix(fpga/intc): Make IrqVectorInfo initialization more explicit

e7812ed

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(kernel): Mark ~Driver interface destructor as virtual

2bdd6e0

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(clang): Allow unkown warning options

e20190a

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(tests/utils): Fix string use-after-free

33c1a13

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(fpga/rtds): Fix broken reinterpret_cast to virtual base class

a03a7eb

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(cpuset): Fix ambiguous C++20 operator== overload

7cee0f3

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(memory): Fix non-virtual destructor in polymorphic class

11e4f11

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(exceptions): Add missing override annotation

8d0780c

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(opal/ddf): Fix non-virtual destructor in polymorphic class

fa748f6

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(webrtc): Fix use of deleted default constructor of sample_pool

a90acbf

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(tests/config): Remove broken putenv call

968474e

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(tests/queue): Fix unintentional sleep(0) using usleep

33be336

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(tests/format): Use proper printf formatting specifiers for std::i…

0e7575f

…nt64_t Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(config): Fix string use-after-free

86aadc3

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(hook/lua): Remove unused lua_tojson function

ba18200

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(hooks/lua): Fix forward declaration of SignalType

1d786d9

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(format/json_kafka): Fix json_pack string parameter

bcd629a

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(sample): Remove C compatability macros

b267998

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(web): Conditionally add global extensions array

d3b2ec2

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(opal-async-param): Close header comment

4a32887

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

fix(treewide): Fix virtual/override annotations

c788b73

Signed-off-by: Philipp Jungkamp <philipp.jungkamp@rwth-aachen.de> Signed-off-by: Steffen Vogel <steffen.vogel@opal-rt.com>

stv0g force-pushed the clang-tidy branch from 4c3d1f9 to c788b73 Compare January 26, 2026 09:11

stv0g approved these changes Jan 26, 2026

View reviewed changes

stv0g mentioned this pull request Jan 26, 2026

Enable Clang builds in CI #995

Open

stv0g merged commit a960050 into master Jan 26, 2026
3 checks passed

stv0g deleted the clang-tidy branch January 26, 2026 09:30

Introduce clang-tidy #920

Introduce clang-tidy #920

Uh oh!

Conversation

pjungkamp commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is clang-tidy

Progress

Uh oh!

pjungkamp commented Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pjungkamp commented Jun 4, 2025

Uh oh!

stv0g commented Jun 5, 2025

Uh oh!

n-eiling left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n-eiling commented Jun 5, 2025

Uh oh!

pjungkamp commented Jun 5, 2025

Uh oh!

stv0g commented Jun 12, 2025

Uh oh!

stv0g commented Oct 30, 2025

Uh oh!

pjungkamp commented Jan 25, 2026

Uh oh!

pjungkamp commented Jan 25, 2026

Uh oh!

pjungkamp commented Jan 25, 2026

Uh oh!

pjungkamp commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stv0g commented Jan 26, 2026

Uh oh!

stv0g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

pjungkamp commented Jun 4, 2025 •

edited

Loading

What is `clang-tidy`

pjungkamp commented Jun 4, 2025 •

edited

Loading

pjungkamp commented Jan 25, 2026 •

edited

Loading