lnls-dig · ericonr · Jan 8, 2025 · Apr 9, 2025 · Apr 9, 2025 · Apr 9, 2025
diff --git a/.clang-tidy b/.clang-tidy
@@ -0,0 +1,19 @@
+Checks: "bugprone*,\
+         cert-*,\
+         clang-analyzer*,\
+         concurrency-*,\
+         modernize-*,\
+         performance-*,\
+         readability-*,\
+         -bugprone-empty-catch,\
+         -concurrency-mt-unsafe,\
+         -modernize-use-trailing-return-type,\
+         -performance-avoid-endl,\
+         -readability-braces-around-statements,\
+         -readability-identifier-length,\
+         -readability-implicit-bool-conversion,\
+         -readability-named-parameter,\
+         "
+
+HeaderFilterRegex: "(util|modules)/[^/]*\\.h"
+WarningsAsErrors: "*"
diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml
@@ -19,6 +19,19 @@ jobs:
       fail-fast: false
       matrix:
         tool:
+          # clang-tidy is more extensive and the list of checks isn't final, so fixing all of them at once isn't tractable;
+          # therefore, the job doesn't fail due to clang-tidy, it's simply informative
+          - packages: "clang-tidy"
+            command: "clang-tidy '--warnings-as-errors=-*' -p build/ $(git ls-files | grep '\\.c\\+$')"
+          # however, we should check that the changed lines of code don't have any tidy warnings
+          - packages: "clang-tidy"
+            command: |
+              if [ -n \"$GITHUB_BASE_REF\" ]; then
+                git diff -U0 origin/$GITHUB_BASE_REF > diff
+                clang-tidy-diff -p1 -config-file .clang-tidy -path build/ < diff > result && exitcode=0 || exitcode=$?
+                cat result
+                exit $exitcode
+              fi
           # cppcheck doesn't check as much, so we can make the job fail because of it;
           # tests are skipped due to issues with Catch2 headers
           - packages: "cppcheck"

diff --git a/app/decode-reg.cc b/app/decode-reg.cc
@@ -1,4 +1,4 @@
-#include <inttypes.h>
+#include <cinttypes>
 
 #include <algorithm>
 #include <chrono>
@@ -280,7 +280,7 @@ int main(int argc, char *argv[])
 
                 if (type == "pos_calc") {
                     /* force module to actually read these registers */
-                    auto dec_pos_calc = dynamic_cast<pos_calc::Core *>(dec.get());
+                    auto *dec_pos_calc = dynamic_cast<pos_calc::Core *>(dec.get());
                     dec_pos_calc->fifo_empty();
                     dec_pos_calc->get_fifo_amps();
                 }

diff --git a/include/modules/afc_timing.h b/include/modules/afc_timing.h
@@ -39,7 +39,7 @@ class Controller: public RegisterDecoderController {
         uint8_t n1, hs_div;
     } rtm_clock = {}, afc_clock = {};
 
-    bool set_freq(double, struct Controller::clock &);
+    static bool set_freq(double, struct Controller::clock &);
 
   public:
     Controller(struct pcie_bars &);

diff --git a/include/modules/fofb_processing.h b/include/modules/fofb_processing.h
@@ -22,7 +22,7 @@ class Core: public RegisterDecoder {
 
   public:
     Core(struct pcie_bars &);
-    ~Core();
+    ~Core() override;
 
     std::vector<int32_t> ref_orb_x, ref_orb_y;
     std::vector<std::vector<double>> coefficients_x, coefficients_y;

diff --git a/include/modules/fofb_shaper_filt.h b/include/modules/fofb_shaper_filt.h
@@ -28,7 +28,7 @@ class Core: public RegisterDecoder {
 
   public:
     Core(struct pcie_bars &);
-    ~Core();
+    ~Core() override;
 
     void print(FILE *, bool) const override;
 

diff --git a/include/modules/lamp.h b/include/modules/lamp.h
@@ -22,7 +22,7 @@ class Core: public RegisterDecoder {
 
   public:
     Core(struct pcie_bars &);
-    ~Core();
+    ~Core() override;
 };
 
 class Controller: public RegisterDecoderController {

diff --git a/include/modules/si57x_ctrl.h b/include/modules/si57x_ctrl.h
@@ -36,7 +36,7 @@ class Controller: public RegisterDecoderController {
     /** The device's startup frequency. */
     double fstartup;
     /** The device's internal crystal frequency. */
-    double fxtal;
+    double fxtal{0};
 
     /** Update all registers and return state of busy flag. */
     bool get_busy();

diff --git a/include/modules/sysid.h b/include/modules/sysid.h
@@ -28,7 +28,7 @@ class Core: public RegisterDecoder {
 
   public:
     Core(struct pcie_bars &);
-    ~Core();
+    ~Core() override;
 
     void print(FILE *, bool) const override;
 

diff --git a/include/modules/trigger_iface.h b/include/modules/trigger_iface.h
@@ -21,7 +21,7 @@ class Core: public RegisterDecoder {
 
   public:
     Core(struct pcie_bars &);
-    ~Core();
+    ~Core() override;
 };
 
 class Controller: public RegisterController {

diff --git a/include/modules/trigger_mux.h b/include/modules/trigger_mux.h
@@ -21,7 +21,7 @@ class Core: public RegisterDecoder {
 
   public:
     Core(struct pcie_bars &);
-    ~Core();
+    ~Core() override;
 };
 
 class Controller: public RegisterController {

diff --git a/modules/acq.cc b/modules/acq.cc
@@ -21,7 +21,7 @@ const size_t acq_ram_per_core = acq_ram / max_acq_cores;
 class MemoryAllocator {
     std::unordered_map<struct pcie_bars *, int> counts;
 
-    MemoryAllocator() {}
+    MemoryAllocator() = default;
 
   public:
     std::array<size_t, 2> get_range(struct pcie_bars &bars)
@@ -175,7 +175,7 @@ void Core::decode()
     /* acquisition channel control */
     t = regs.acq_chan_ctl;
     /* will be used to determine how many channels to show in the next block */
-    unsigned num_chan = extract_value<uint32_t>(t, ACQ_CORE_ACQ_CHAN_CTL_NUM_CHAN_MASK);
+    auto num_chan = extract_value<uint32_t>(t, ACQ_CORE_ACQ_CHAN_CTL_NUM_CHAN_MASK);
     add_general("WHICH", extract_value<uint32_t>(t, ACQ_CORE_ACQ_CHAN_CTL_WHICH_MASK));
     add_general("DTRIG_WHICH", extract_value<uint32_t>(t, ACQ_CORE_ACQ_CHAN_CTL_DTRIG_WHICH_MASK));
     add_general("NUM_CHAN", num_chan);
@@ -191,7 +191,8 @@ void Core::decode()
     memcpy(p, &regs.ch0_desc, sizeof p);
 
     for (unsigned i = 0; i < num_chan; i++) {
-        uint32_t desc = p[i*REGISTERS_PER_CHAN], adesc = p[i*REGISTERS_PER_CHAN + 1];
+        uint32_t desc = p[i*REGISTERS_PER_CHAN];
+        uint32_t adesc = p[(i*REGISTERS_PER_CHAN) + 1];
         add_channel("INT_WIDTH", i, extract_value<uint32_t>(desc, ACQ_CORE_CH0_DESC_INT_WIDTH_MASK));
         add_channel("NUM_COALESCE", i, extract_value<uint32_t>(desc, ACQ_CORE_CH0_DESC_NUM_COALESCE_MASK));
         add_channel("NUM_ATOMS", i, extract_value<uint32_t>(adesc, ACQ_CORE_CH0_ATOM_DESC_NUM_ATOMS_MASK));
@@ -237,9 +238,9 @@ struct NoSamples: std::runtime_error {
 
 void Controller::get_internal_values()
 {
-    uint32_t channel_desc = bar4_read(&bars, addr + ACQ_CORE_CH0_DESC + 8*channel);
-    uint32_t num_coalesce = extract_value<uint32_t>(channel_desc, ACQ_CORE_CH0_DESC_NUM_COALESCE_MASK);
-    uint32_t int_width = extract_value<uint32_t>(channel_desc, ACQ_CORE_CH0_DESC_INT_WIDTH_MASK);
+    uint32_t channel_desc = bar4_read(&bars, addr + ACQ_CORE_CH0_DESC + (8*channel));
+    auto num_coalesce = extract_value<uint32_t>(channel_desc, ACQ_CORE_CH0_DESC_NUM_COALESCE_MASK);
+    auto int_width = extract_value<uint32_t>(channel_desc, ACQ_CORE_CH0_DESC_INT_WIDTH_MASK);
 
     /* int_width is in bits, so needs to be converted to bytes */
     sample_size = (int_width / 8) * num_coalesce;
@@ -248,7 +249,7 @@ void Controller::get_internal_values()
 
     alignment = (ddr3_payload_size > sample_size) ? ddr3_payload_size / sample_size : 1;
 
-    uint32_t channel_atom_desc = bar4_read(&bars, addr + ACQ_CORE_CH0_ATOM_DESC + 8*channel);
+    uint32_t channel_atom_desc = bar4_read(&bars, addr + ACQ_CORE_CH0_ATOM_DESC + (8*channel));
     channel_atom_width = extract_value<uint32_t>(channel_atom_desc, ACQ_CORE_CH0_ATOM_DESC_ATOM_WIDTH_MASK);
     channel_num_atoms = extract_value<uint32_t>(channel_atom_desc, ACQ_CORE_CH0_ATOM_DESC_NUM_ATOMS_MASK);
 
@@ -267,14 +268,13 @@ void Controller::encode_params()
     auto align_extend = [](unsigned value, unsigned alignment, bool can_be_zero) -> unsigned {
         if (value == 0) {
             if (can_be_zero) return 0;
-            else return alignment;
+            return alignment;
         }
 
         unsigned extra = value % alignment;
         if (extra)
             return value + (alignment - extra);
-        else
-            return value;
+        return value;
     };
 
     if (post_samples + pre_samples == 0)
@@ -302,7 +302,7 @@ void Controller::encode_params()
         {"data", {false, true, false, false}},
         {"software", {false, false, true, false}},
     });
-    auto &trigger_setting = trigger_types.at(trigger_type);
+    const auto &trigger_setting = trigger_types.at(trigger_type);
     insert_bit(regs.ctl, trigger_setting[0], ACQ_CORE_CTL_FSM_ACQ_NOW);
     insert_bit(regs.trig_cfg, trigger_setting[1], ACQ_CORE_TRIG_CFG_HW_TRIG_EN);
     insert_bit(regs.trig_cfg, trigger_setting[2], ACQ_CORE_TRIG_CFG_SW_TRIG_EN);
@@ -381,8 +381,8 @@ std::vector<Data> Controller::get_result()
     m_step = acq_step::stop;
 
     /* total number of elements (samples*atoms) */
-    size_t total_samples = acq_pre_samples + acq_post_samples,
-           elements = total_samples * channel_num_atoms;
+    size_t total_samples = acq_pre_samples + acq_post_samples;
+    size_t elements = total_samples * channel_num_atoms;
     size_t total_bytes = elements * (channel_atom_width/8);
 
     /* this is an identity, just want to be sure */
@@ -422,8 +422,8 @@ std::vector<Data> Controller::get_result()
      * amount of samples */
     auto bytes2samples = [this](ssize_t v) -> ssize_t { return v / sample_size; };
     auto samples2bytes = [this](ssize_t v) -> ssize_t { return v * sample_size; };
-    const ssize_t max_bytes = ram_end_addr - ram_start_addr,
-          max_samples = bytes2samples(max_bytes);
+    const ssize_t max_bytes = ram_end_addr - ram_start_addr;
+    const ssize_t max_samples = bytes2samples(max_bytes);
     /* in order to simplify working with the acquisition circular buffer, think
      * first in terms of indexes into a circular buffer */
     const ssize_t trigger_index = bytes2samples(trigger_pos - ram_start_addr);
@@ -523,7 +523,7 @@ void Controller::print_csv(FILE *f, std::vector<T> &res)
     for (unsigned i = 0; i < (pre_samples + post_samples); i++) {
         for (unsigned j = 0; j < channel_num_atoms; j++) {
             char tmp[32];
-            auto r = std::to_chars(tmp, tmp + sizeof tmp, res[i * channel_num_atoms + j]);
+            auto r = std::to_chars(tmp, tmp + sizeof tmp, res[(i * channel_num_atoms) + j]);
             *r.ptr = '\0';
             fputs(tmp, f);
             fputc(',', f);

diff --git a/modules/afc_timing.cc b/modules/afc_timing.cc
@@ -134,7 +134,7 @@ void Core::decode()
     add_general("ALIVE", regs.alive);
 
     size_t i = 0;
-    for (auto clockp: {&regs.rtm_clock, &regs.afc_clock}) {
+    for (auto *clockp: {&regs.rtm_clock, &regs.afc_clock}) {
         add_channel("RFREQ_HI", i, clockp->rfreq_hi);
         add_channel("RFREQ_LO", i, clockp->rfreq_lo);
         add_channel("N1", i, extract_value<uint8_t>(clockp->n1_hs_div, TIMING_RTM_N1_MASK));

diff --git a/modules/fofb_processing.cc b/modules/fofb_processing.cc
@@ -1,5 +1,6 @@
 #include <algorithm>
 #include <cstring>
+#include <ranges>
 #include <stdexcept>
 #include <type_traits>
 
@@ -68,13 +69,13 @@ void Core::read_monitors()
         get_register(
             WB_FOFB_PROCESSING_REGS_CH +
             WB_FOFB_PROCESSING_REGS_CH_SP_DECIM_DATA +
-            i * WB_FOFB_PROCESSING_REGS_CH_SIZE);
+            (i * WB_FOFB_PROCESSING_REGS_CH_SIZE));
 }
 
 void Core::decode()
 {
-    uint32_t fixed_point_gain = extract_value<uint32_t>(regs.fixed_point_pos.accs_gains, WB_FOFB_PROCESSING_REGS_FIXED_POINT_POS_ACCS_GAINS_VAL_MASK);
-    uint32_t fixed_point_coeff = extract_value<uint32_t>(regs.fixed_point_pos.coeff, WB_FOFB_PROCESSING_REGS_FIXED_POINT_POS_COEFF_VAL_MASK);
+    auto fixed_point_gain = extract_value<uint32_t>(regs.fixed_point_pos.accs_gains, WB_FOFB_PROCESSING_REGS_FIXED_POINT_POS_ACCS_GAINS_VAL_MASK);
+    auto fixed_point_coeff = extract_value<uint32_t>(regs.fixed_point_pos.coeff, WB_FOFB_PROCESSING_REGS_FIXED_POINT_POS_COEFF_VAL_MASK);
     add_general("FIXED_POINT_POS_GAINS", fixed_point_gain);
     add_general("FIXED_POINT_POS_COEFF", fixed_point_coeff);
 
@@ -97,19 +98,17 @@ void Core::decode()
     coefficients_x.resize(MAX_NUM_CHAN);
     coefficients_y.resize(MAX_NUM_CHAN);
 
-    /* XXX: use C++20's std::ranges::generate when available */
-
     for (unsigned i = 0; i < *number_of_channels; i++) {
         auto &ram_bank = regs.ch[i].coeff_ram_bank;
         const size_t elements = MAX_BPMS;
         coefficients_x[i].resize(elements);
         coefficients_y[i].resize(elements);
 
         size_t u = 0;
-        std::generate(coefficients_x[i].begin(), coefficients_x[i].end(),
+        std::ranges::generate(coefficients_x[i],
             [&](){ return fixed2float(ram_bank[u++].data, fixed_point_coeff); });
         u = MAX_BPMS;
-        std::generate(coefficients_y[i].begin(), coefficients_y[i].end(),
+        std::ranges::generate(coefficients_y[i],
             [&](){ return fixed2float(ram_bank[u++].data, fixed_point_coeff); });
 
         add_channel("CH_ACC_CTL_FREEZE", i, get_bit(regs.ch[i].acc.ctl, WB_FOFB_PROCESSING_REGS_CH_ACC_CTL_FREEZE));
@@ -121,9 +120,9 @@ void Core::decode()
     }
 
     size_t u = 0;
-    std::generate(ref_orb_x.begin(), ref_orb_x.end(), [&](){ return (int32_t)regs.sps_ram_bank[u++].data; });
+    std::ranges::generate(ref_orb_x, [&](){ return (int32_t)regs.sps_ram_bank[u++].data; });
     u = MAX_BPMS; /* access the second half of the RAM bank */
-    std::generate(ref_orb_y.begin(), ref_orb_y.end(), [&](){ return (int32_t)regs.sps_ram_bank[u++].data; });
+    std::ranges::generate(ref_orb_y, [&](){ return (int32_t)regs.sps_ram_bank[u++].data; });
 }
 
 void Core::print(FILE *f, bool verbose) const

diff --git a/modules/fofb_shaper_filt.cc b/modules/fofb_shaper_filt.cc
@@ -76,8 +76,8 @@ void Core::decode()
     for (unsigned i = 0; i < NUM_CHANNELS; i++) {
         for (unsigned j = 0; j < num_biquads; j++) {
             for (unsigned k = 0; k < COEFFS_PER_BIQUAD; k++) {
-                coefficients.values[i][k + COEFFS_PER_BIQUAD * j] =
-                    fixed2float(regs.ch[i].coeffs[k + TOTAL_PER_BIQUAD * j].val, fixed_point_coeff);
+                coefficients.values[i][k + (COEFFS_PER_BIQUAD * j)] =
+                    fixed2float(regs.ch[i].coeffs[k + (TOTAL_PER_BIQUAD * j)].val, fixed_point_coeff);
             }
         }
     }
@@ -128,8 +128,8 @@ void Controller::encode_params()
     for (unsigned i = 0; i < NUM_CHANNELS; i++) {
         for (unsigned j = 0; j < num_biquads; j++) {
             for (unsigned k = 0; k < COEFFS_PER_BIQUAD; k++) {
-                regs.ch[i].coeffs[k + TOTAL_PER_BIQUAD * j].val =
-                    float2fixed(coefficients.values[i][k + COEFFS_PER_BIQUAD * j], fixed_point_coeff);
+                regs.ch[i].coeffs[k + (TOTAL_PER_BIQUAD * j)].val =
+                    float2fixed(coefficients.values[i][k + (COEFFS_PER_BIQUAD * j)], fixed_point_coeff);
             }
         }
     }
@@ -143,7 +143,7 @@ void Controller::write_params()
         for (unsigned j = 0; j < num_biquads; j++) {
             bar4_write_v(
                 &bars,
-                addr + WB_FOFB_SHAPER_FILT_REGS_CH_COEFFS + i * sizeof(regs.ch[0]) + TOTAL_PER_BIQUAD * j * sizeof(uint32_t),
+                addr + WB_FOFB_SHAPER_FILT_REGS_CH_COEFFS + (i * sizeof(regs.ch[0])) + (TOTAL_PER_BIQUAD * j * sizeof(uint32_t)),
                 &regs.ch[i].coeffs[TOTAL_PER_BIQUAD * j].val,
                 COEFFS_PER_BIQUAD * sizeof(uint32_t));
         }

diff --git a/modules/isla216p.cc b/modules/isla216p.cc
@@ -68,6 +68,7 @@ bool Controller::set_reg(uint8_t addr, uint8_t rdata, spi::Channel channel)
 
 bool Controller::set_defaults(spi::Channel)
 {
+    (void)this;
     return false;
 }
 

diff --git a/modules/lamp.cc b/modules/lamp.cc
@@ -10,8 +10,8 @@ namespace lamp {
 #include "hw/wb_rtmlamp_ohwr_regs.h"
 
 namespace {
-    static constexpr unsigned NUM_CHAN = 12;
-    static constexpr unsigned TRIGGER_ENABLE_VERSION = 1;
+    constexpr unsigned NUM_CHAN = 12;
+    constexpr unsigned TRIGGER_ENABLE_VERSION = 1;
 
     constexpr unsigned LAMP_DEVID = 0xa1248bec;
     struct sdb_device_info ref_devinfo = {
@@ -88,7 +88,8 @@ Core::~Core() = default;
 
 void Core::decode()
 {
-    uint32_t t, *pt;
+    uint32_t t;
+    uint32_t *pt;
 
     /* add printer if this value ever gets flags;
      * this is being done for IOC compatibility */

diff --git a/modules/pos_calc.cc b/modules/pos_calc.cc
@@ -51,17 +51,17 @@ struct SyncMaskRates {
     } tbt, facq, monit;
     SyncMaskRates(struct pos_calc &regs):
         tbt{
-            &regs.tbt_tag,
-            &regs.tbt_data_mask_ctl,
-            &regs.tbt_data_mask_samples,},
+            .tag = &regs.tbt_tag,
+            .data_mask_ctl = &regs.tbt_data_mask_ctl,
+            .data_mask_samples = &regs.tbt_data_mask_samples,},
         facq{
-            &regs.monit1_tag,
-            &regs.monit1_data_mask_ctl,
-            &regs.monit1_data_mask_samples,},
+            .tag = &regs.monit1_tag,
+            .data_mask_ctl = &regs.monit1_data_mask_ctl,
+            .data_mask_samples = &regs.monit1_data_mask_samples,},
         monit{
-            &regs.monit_tag,
-            &regs.monit_data_mask_ctl,
-            &regs.monit_data_mask_samples,}
+            .tag = &regs.monit_tag,
+            .data_mask_ctl = &regs.monit_data_mask_ctl,
+            .data_mask_samples = &regs.monit_data_mask_samples,}
     {
     }
 };
-Original file line number
+Diff line change
@@ Expand Up @@
     bool Controller::set_defaults(spi::Channel)
     {
+        (void)this;
         return false;
     }
@@ Expand Down @@