Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .clang-tidy
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# SPDX-FileCopyrightText: 2025 Institute for Automation of Complex Power Systems, RWTH Aachen University
# SPDX-License-Identifier: Apache-2.0

Checks: >
-*,
modernize-use-override,
misc-const-correctness,
WarningsAsErrors: '*'
HeaderFilterRegex: villas/.*
FormatStyle: file
1 change: 1 addition & 0 deletions .envrc
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
# SPDX-License-Identifier: Apache-2.0

use flake
pre-commit install
8 changes: 7 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,13 @@ include(FindSymbol)
include(CMakeDependentOption)

add_definitions(-D_POSIX_C_SOURCE=200809L -D_GNU_SOURCE)
add_compile_options(-Wall -Wno-unknown-pragmas -fdiagnostics-color=auto)
add_compile_options(
-Wall
-Wno-unknown-pragmas
-Wno-vla-cxx-extension
-Wno-unused-command-line-argument
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you motivate why you introduced these new options?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCC and Clang have a different set of warnings enabled with -Wall. This silences two warnings that would, together with -Werror break a Clang Build.

Copy link
Member

@n-eiling n-eiling Jun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't fixing them be more desireable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced two new flags here.

  • -Wno-vla-cxx-extension: Clang would issue a warning for each variable length array on the stack. This affects most node types.
  • -Wno-unused-command-line-argument: CMake seems to introduce -Wl,--compress-debug-sections command line arguments which Clang doesn't understand and then warns on.

-fdiagnostics-color=auto
)

# Check OS
check_include_file("sys/eventfd.h" HAS_EVENTFD)
Expand Down
14 changes: 7 additions & 7 deletions clients/shmem/villas-shmem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class Shmem : public Tool {
protected:
std::atomic<bool> stop;

void usage() {
void usage() override {
std::cout
<< "Usage: villas-test-shmem WNAME VECTORIZE" << std::endl
<< " WNAME name of the shared memory object for the output queue"
Expand All @@ -47,9 +47,9 @@ class Shmem : public Tool {
printCopyright();
}

void handler(int, siginfo_t *, void *) { stop = true; }
void handler(int, siginfo_t *, void *) override { stop = true; }

int main() {
int main() override {
int ret, readcnt, writecnt, avail;

struct ShmemInterface shm;
Expand All @@ -62,9 +62,9 @@ class Shmem : public Tool {
return 1;
}

std::string wname = argv[1];
std::string rname = argv[2];
int vectorize = atoi(argv[3]);
std::string const wname = argv[1];
std::string const rname = argv[2];
int const vectorize = atoi(argv[3]);

ret = shmem_int_open(wname.c_str(), rname.c_str(), &shm, &conf);
if (ret < 0)
Expand All @@ -87,7 +87,7 @@ class Shmem : public Tool {
outsmps[i]->sequence = insmps[i]->sequence;
outsmps[i]->ts = insmps[i]->ts;

int len = MIN(insmps[i]->length, outsmps[i]->capacity);
int const len = MIN(insmps[i]->length, outsmps[i]->capacity);
memcpy(outsmps[i]->data, insmps[i]->data, SAMPLE_DATA_LENGTH(len));

outsmps[i]->length = len;
Expand Down
2 changes: 1 addition & 1 deletion common/include/villas/cpuset.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ class CpuSet {
size_t size() const { return sz; }

CpuSet operator~() {
CpuSet full = UINTMAX_MAX;
CpuSet const full = UINTMAX_MAX;

return full ^ *this;
}
Expand Down
2 changes: 1 addition & 1 deletion common/include/villas/dsp/window_cosine.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class CosineWindow : public Window<T> {

T correctionFactor;

virtual T filter(T in, size_type i) const { return in * coefficients[i]; }
T filter(T in, size_type i) const override { return in * coefficients[i]; }

public:
CosineWindow(double a0, double a1, double a2, double a3, double a4,
Expand Down
6 changes: 3 additions & 3 deletions common/include/villas/exceptions.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ class ConfigError : public std::runtime_error {
}

public:
~ConfigError() {
~ConfigError() override {
if (msg)
free(msg);
}
Expand Down Expand Up @@ -132,12 +132,12 @@ class ConfigError : public std::runtime_error {
}

std::string docUri() const {
std::string baseUri = "https://villas.fein-aachen.org/doc/jump?";
std::string const baseUri = "https://villas.fein-aachen.org/doc/jump?";

return baseUri + id;
}

virtual const char *what() const noexcept { return msg; }
const char *what() const noexcept override { return msg; }
};

} // namespace villas
2 changes: 1 addition & 1 deletion common/include/villas/graph/directed.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class DirectedGraph {
EdgeIdentifier addDefaultEdge(VertexIdentifier fromVertexId,
VertexIdentifier toVertexId) {
// Create a new edge
std::shared_ptr<EdgeType> edge(new EdgeType);
std::shared_ptr<EdgeType> const edge(new EdgeType);

return addEdge(edge, fromVertexId, toVertexId);
}
Expand Down
1 change: 1 addition & 0 deletions common/include/villas/kernel/devices/driver.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class Driver {
virtual std::string name() const = 0;
virtual void override(const Device &device) const = 0;
virtual void unbind(const Device &device) const = 0;
virtual ~Driver() {}
};

} // namespace devices
Expand Down
4 changes: 1 addition & 3 deletions common/include/villas/kernel/vfio_device.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@

#pragma once

#include <list>
#include <memory>
#include <string>
#include <vector>

Expand Down Expand Up @@ -99,7 +97,7 @@ class Device {
std::vector<void *> mappings;

// libpci handle of the device
const kernel::devices::PciDevice *pci_device;
[[maybe_unused]] const kernel::devices::PciDevice *pci_device;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Logger log;
};
Expand Down
16 changes: 9 additions & 7 deletions common/include/villas/memory.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ template <typename DerivedAllocator> class BaseAllocator {
: memoryAddrSpaceId(memoryAddrSpaceId) {
// CRTP
derivedAlloc = static_cast<DerivedAllocator *>(this);
std::string loggerName = fmt::format("memory:", derivedAlloc->getName());
std::string const loggerName =
fmt::format("memory:", derivedAlloc->getName());
logger = Log::get(loggerName);

// Default deallocation callback
Expand All @@ -136,7 +137,6 @@ template <typename DerivedAllocator> class BaseAllocator {
}

virtual std::unique_ptr<MemoryBlock, MemoryBlock::deallocator_fn>

allocateBlock(size_t size) = 0;

template <typename T> MemoryAccessor<T> allocate(size_t num) {
Expand All @@ -152,7 +152,7 @@ template <typename DerivedAllocator> class BaseAllocator {
// Check if the allocated memory is really accessible by writing to the
// allocated memory and reading back. Exponentially increase offset to
// speed up testing.
MemoryAccessor<volatile uint8_t> byteAccessor(*mem);
MemoryAccessor<volatile uint8_t> const byteAccessor(*mem);
size_t idx = 0;
for (int i = 0; idx < mem->getSize(); i++, idx = (1 << i)) {
auto val = static_cast<uint8_t>(i);
Expand All @@ -166,6 +166,8 @@ template <typename DerivedAllocator> class BaseAllocator {
return MemoryAccessor<T>(std::move(mem));
}

virtual ~BaseAllocator() {}

protected:
void insertMemoryBlock(const MemoryBlock &mem) {
auto &mm = MemoryManager::get();
Expand Down Expand Up @@ -217,8 +219,8 @@ class LinearAllocator : public BaseAllocator<LinearAllocator> {

std::string getName() const;

virtual std::unique_ptr<MemoryBlock, MemoryBlock::deallocator_fn>
allocateBlock(size_t size);
std::unique_ptr<MemoryBlock, MemoryBlock::deallocator_fn>
allocateBlock(size_t size) override;

private:
static constexpr size_t alignBytes = sizeof(uintptr_t);
Expand Down Expand Up @@ -247,7 +249,7 @@ class HostRam {
std::string getName() const { return "HostRamAlloc"; }

std::unique_ptr<MemoryBlock, MemoryBlock::deallocator_fn>
allocateBlock(size_t size);
allocateBlock(size_t size) override;
};

static HostRamAllocator &getAllocator() {
Expand All @@ -264,7 +266,7 @@ class HostDmaRam {
public:
HostDmaRamAllocator(int num);

virtual ~HostDmaRamAllocator();
~HostDmaRamAllocator() override;

std::string getName() const { return getUdmaBufName(num); }

Expand Down
10 changes: 5 additions & 5 deletions common/include/villas/popen.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ class PopenStream : public Popen {

std::ostream &cout() { return *(output.stream); }

template <typename T>
friend std::istream &operator>>(PopenStream &po, T &value) {
return *(po.input.stream) >> value;
}

void open();
int close();

Expand All @@ -84,8 +89,3 @@ class PopenStream : public Popen {

} // namespace utils
} // namespace villas

template <typename T>
std::istream &operator>>(villas::utils::PopenStream &po, T &value) {
return *(po.input.stream) >> value;
}
8 changes: 4 additions & 4 deletions common/include/villas/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@
#endif
#define MAX(a, b) \
({ \
__typeof__(a) _a = (a); \
__typeof__(b) _b = (b); \
__typeof__(a) const &_a = (a); \
__typeof__(b) const &_b = (b); \
_a > _b ? _a : _b; \
})

Expand All @@ -92,8 +92,8 @@
#endif
#define MIN(a, b) \
({ \
__typeof__(a) _a = (a); \
__typeof__(b) _b = (b); \
__typeof__(a) const &_a = (a); \
__typeof__(b) const &_b = (b); \
_a < _b ? _a : _b; \
})
#define MIN3(a, b, c) MIN(MIN((a), (b)), (c))
Expand Down
10 changes: 5 additions & 5 deletions common/lib/dsp/pid.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,18 @@ PID::PID(double _dt, double _max, double _min, double _Kp, double _Kd,

double PID::calculate(double setpoint, double pv) {
// Calculate error
double error = setpoint - pv;
double const error = setpoint - pv;

// Proportional term
double Pout = Kp * error;
double const Pout = Kp * error;

// Integral term
integral += error * dt;
double Iout = Ki * integral;
double const Iout = Ki * integral;

// Derivative term
double derivative = (error - pre_error) / dt;
double Dout = Kd * derivative;
double const derivative = (error - pre_error) / dt;
double const Dout = Kd * derivative;

// Calculate total output
double output = Pout + Iout + Dout;
Expand Down
12 changes: 6 additions & 6 deletions common/lib/hist.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ void Hist::put(double value) {
// There is no warmup phase
// TODO resolution = ?
} else {
idx_t idx = std::round((value - low) / resolution);
idx_t const idx = std::round((value - low) / resolution);

// Check bounds and increment
if (idx >= (idx_t)data.size())
Expand Down Expand Up @@ -123,7 +123,7 @@ void Hist::print(Logger logger, bool details, std::string prefix) const {

void Hist::plot(Logger logger) const {
// Get highest bar
Hist::cnt_t max = *std::max_element(data.begin(), data.end());
Hist::cnt_t const max = *std::max_element(data.begin(), data.end());

std::vector<TableColumn> cols = {
{-9, TableColumn::Alignment::RIGHT, "Value", "%+9.3g"},
Expand All @@ -136,9 +136,9 @@ void Hist::plot(Logger logger) const {
table.header();

for (size_t i = 0; i < data.size(); i++) {
double value = low + (i)*resolution;
Hist::cnt_t cnt = data[i];
int bar = cols[2].getWidth() * ((double)cnt / max);
double const value = low + (i)*resolution;
Hist::cnt_t const cnt = data[i];
int const bar = cols[2].getWidth() * ((double)cnt / max);

char *buf = strf("%s", "");
for (int i = 0; i < bar; i++)
Expand Down Expand Up @@ -196,7 +196,7 @@ json_t *Hist::toJson() const {
int Hist::dumpJson(FILE *f) const {
json_t *j = Hist::toJson();

int ret = json_dumpf(j, f, 0);
int const ret = json_dumpf(j, f, 0);

json_decref(j);

Expand Down
6 changes: 3 additions & 3 deletions common/lib/kernel/devices/device_connection.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ DeviceConnection DeviceConnection::from(
auto logger = villas::Log::get("Builder: DeviceConnection");

// Bind the devicetree device to vfio driver
LinuxDriver driver(
LinuxDriver const driver(
std::filesystem::path("/sys/bus/platform/drivers/vfio-platform"));
driver.attach(device);

Expand Down Expand Up @@ -55,9 +55,9 @@ void DeviceConnection::addToMemorygraph() const {

// Create memorygraph edge from process to vfio device
auto &mm = MemoryManager::get();
size_t srcVertexId = mm.getProcessAddressSpace();
size_t const srcVertexId = mm.getProcessAddressSpace();
const size_t mem_size = this->vfio_device->regionGetSize(0);
size_t targetVertexId =
size_t const targetVertexId =
mm.getOrCreateAddressSpace(this->vfio_device->getName());
mm.createMapping(reinterpret_cast<uintptr_t>(mapping), 0, mem_size,
"process to vfio", srcVertexId, targetVertexId);
Expand Down
8 changes: 4 additions & 4 deletions common/lib/kernel/devices/ip_device.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ IpDevice IpDevice::from(const std::filesystem::path unsafe_path) {
}

std::string IpDevice::ip_name() const {
int pos = name().find('.');
int const pos = name().find('.');
return name().substr(pos + 1);
}

size_t IpDevice::addr() const {
size_t pos = name().find('.');
std::string addr_hex = name().substr(0, pos);
size_t const pos = name().find('.');
std::string const addr_hex = name().substr(0, pos);

// Convert from hex-string to number
std::stringstream ss;
Expand All @@ -43,7 +43,7 @@ size_t IpDevice::addr() const {
}

bool IpDevice::is_path_valid(const std::filesystem::path unsafe_path) {
std::string assumed_device_name = unsafe_path.filename();
std::string const assumed_device_name = unsafe_path.filename();

// Match format of hexaddr.devicename
if (!std::regex_match(assumed_device_name,
Expand Down
2 changes: 1 addition & 1 deletion common/lib/kernel/devices/linux_driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ void LinuxDriver::bind(const Device &device) const {
}

std::string LinuxDriver::name() const {
size_t pos = path.u8string().rfind('/');
size_t const pos = path.u8string().rfind('/');
return path.u8string().substr(pos + 1);
}

Expand Down
Loading