Skip to content

Commit 1e1bdc6

Browse files
CameronBrooks11CameronBrooks11
authored
refactor(clang): adopt C++20 std::format for diagnostics + clang hygiene (#66)
Address the clang-tidy backlog properly rather than with += churn or a check-smothering rule: - Bump to C++20 and convert diagnostic/error-message construction from operator+ chains to std::format (standard, no third-party dep, readable and allocation-efficient; compile-time checked). Covers the graph compiler, dimensional validation, and the YAML/JSON loaders. - Add tests/.clang-tidy relaxing test-noise checks (gtest optional-access false positive, narrowing of literals, perf in fixtures) via the directory-scoped mechanism instead of per-line NOLINTs. - Genuine fixes: delay.hpp uses std::llround instead of the +0.5 idiom; StateSpaceSisoDiscreteModel takes a_d by const ref (was copied); namespace_test.cpp includes <algorithm>/<string> it uses directly. Refs anolishq/.github#79, anolishq/.github#84
1 parent 695d76d commit 1e1bdc6

10 files changed

Lines changed: 138 additions & 120 deletions

File tree

‎CMakeLists.txt‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,8 @@ project(fluxgraph VERSION 1.0.5 LANGUAGES CXX)
3030
include(GNUInstallDirs)
3131
include(CMakePackageConfigHelpers)
3232

33-
# C++17 required
34-
set(CMAKE_CXX_STANDARD 17)
33+
# C++20 required (std::format for diagnostics; standard, no third-party dep)
34+
set(CMAKE_CXX_STANDARD 20)
3535
set(CMAKE_CXX_STANDARD_REQUIRED ON)
3636
set(CMAKE_CXX_EXTENSIONS OFF)
3737

‎include/fluxgraph/model/state_space_siso_discrete.hpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,8 +20,8 @@ namespace fluxgraph {
2020
/// - output_signal: user-defined scalar unit (strict mode requires declaration)
2121
class StateSpaceSisoDiscreteModel : public IModel {
2222
public:
23-
StateSpaceSisoDiscreteModel(const std::string &id, std::vector<std::vector<double>> a_d, std::vector<double> b_d,
24-
std::vector<double> c, double d, std::vector<double> x0,
23+
StateSpaceSisoDiscreteModel(const std::string &id, const std::vector<std::vector<double>> &a_d,
24+
std::vector<double> b_d, std::vector<double> c, double d, std::vector<double> x0,
2525
const std::string &output_signal_path, const std::string &input_signal_path,
2626
SignalNamespace &ns);
2727

‎include/fluxgraph/transform/delay.hpp‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
#pragma once
22

3+
#include <cmath>
4+
#include <cstddef>
35
#include <deque>
46

57
#include "fluxgraph/transform/interface.hpp"
@@ -18,7 +20,7 @@ class DelayTransform : public ITransform {
1820
}
1921

2022
// Compute required buffer size (samples needed for delay)
21-
size_t required_samples = static_cast<size_t>(delay_sec_ / dt + 0.5);
23+
size_t required_samples = static_cast<size_t>(std::llround(delay_sec_ / dt));
2224
if (required_samples == 0) {
2325
required_samples = 1;
2426
}

‎src/graph/compiler.cpp‎

Lines changed: 74 additions & 85 deletions
Large diffs are not rendered by default.

‎src/graph/compiler/dimensional.cpp‎

Lines changed: 20 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
#include "dimensional.hpp"
22

3+
#include <format>
34
#include <stdexcept>
45

56
#include "common.hpp"
@@ -45,23 +46,24 @@ void validate_model_signature_contracts(const ModelSpec &model_spec, const Model
4546
}
4647

4748
const std::string path =
48-
as_string(param_it->second, "model[" + model_spec.id + ":" + model_spec.type + "]/{" + param_name + "}");
49+
as_string(param_it->second, std::format("model[{}:{}]/{{{}}}", model_spec.id, model_spec.type, param_name));
4950
const SignalId id = signal_ns.intern(path);
5051
const std::string actual_unit = resolve_signal_contract_or_empty(signal_contracts, id);
5152

5253
if (actual_unit.empty()) {
5354
if (strict) {
5455
throw std::runtime_error(
55-
"GraphCompiler: strict mode requires declared signal contract "
56-
"for model '" +
57-
model_spec.id + "' parameter '" + param_name + "' (path '" + path + "')");
56+
std::format("GraphCompiler: strict mode requires declared signal contract for model '{}' "
57+
"parameter '{}' (path '{}')",
58+
model_spec.id, param_name, path));
5859
}
5960
continue;
6061
}
6162

6263
if (!expected_unit.empty() && actual_unit != expected_unit) {
63-
const std::string message = "GraphCompiler: model '" + model_spec.id + "' parameter '" + param_name +
64-
"' expects unit '" + expected_unit + "' but found '" + actual_unit + "'";
64+
const std::string message =
65+
std::format("GraphCompiler: model '{}' parameter '{}' expects unit '{}' but found '{}'", model_spec.id,
66+
param_name, expected_unit, actual_unit);
6567
if (strict) {
6668
throw std::runtime_error(message);
6769
}
@@ -71,26 +73,27 @@ void validate_model_signature_contracts(const ModelSpec &model_spec, const Model
7173

7274
for (const auto &[param_name, param_signature] : signature.scalar_param_signatures) {
7375
auto param_it = model_spec.params.find(param_name);
74-
const std::string path = "model[" + model_spec.id + ":" + model_spec.type + "]/" + param_name;
76+
const std::string path = std::format("model[{}:{}]/{}", model_spec.id, model_spec.type, param_name);
7577

7678
if (param_it == model_spec.params.end()) {
7779
if (strict && param_signature.required) {
7880
throw std::runtime_error(
79-
"GraphCompiler: strict mode requires scalar "
80-
"parameter '" +
81-
param_name + "' for model '" + model_spec.id + "'");
81+
std::format("GraphCompiler: strict mode requires scalar parameter '{}' for model '{}'", param_name,
82+
model_spec.id));
8283
}
8384
if (!strict && param_signature.required) {
84-
emit_warning(options, "GraphCompiler: missing required scalar parameter '" + param_name +
85-
"' for model '" + model_spec.id + "'");
85+
emit_warning(options,
86+
std::format("GraphCompiler: missing required scalar parameter '{}' for model '{}'",
87+
param_name, model_spec.id));
8688
}
8789
continue;
8890
}
8991

9092
if (!param_signature.unit_symbol.empty() && !is_unit_known(unit_registry, param_signature.unit_symbol)) {
91-
const std::string message = "GraphCompiler: model signature for type '" + model_spec.type +
92-
"' references unknown scalar unit symbol '" + param_signature.unit_symbol +
93-
"' for parameter '" + param_name + "'";
93+
const std::string message = std::format(
94+
"GraphCompiler: model signature for type '{}' references unknown scalar unit symbol '{}' "
95+
"for parameter '{}'",
96+
model_spec.type, param_signature.unit_symbol, param_name);
9497
if (strict) {
9598
throw std::runtime_error(message);
9699
}
@@ -99,8 +102,8 @@ void validate_model_signature_contracts(const ModelSpec &model_spec, const Model
99102

100103
const double value = as_double(param_it->second, path);
101104
if (!satisfies_scalar_constraint(value, param_signature.constraint)) {
102-
const std::string message = "Invalid parameter at " + path + ": expected " +
103-
format_scalar_constraint_rule(param_signature.constraint);
105+
const std::string message = std::format("Invalid parameter at {}: expected {}", path,
106+
format_scalar_constraint_rule(param_signature.constraint));
104107
if (strict) {
105108
throw std::runtime_error(message);
106109
}

‎src/loaders/json_loader.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
#include "fluxgraph/loaders/json_loader.hpp"
44

5+
#include <format>
56
#include <fstream>
67
#include <nlohmann/json.hpp>
78
#include <stdexcept>
@@ -26,7 +27,7 @@ ParamValue json_to_param_value(const json &j, const std::string &path, detail::P
2627
} else if (j.is_boolean()) {
2728
return j.get<bool>();
2829
} else if (j.is_string()) {
29-
const std::string value = j.get<std::string>();
30+
std::string value = j.get<std::string>();
3031
detail::check_string_size(value.size(), path);
3132
return value;
3233
} else if (j.is_array()) {
@@ -42,7 +43,7 @@ ParamValue json_to_param_value(const json &j, const std::string &path, detail::P
4243
ParamObject obj;
4344
for (auto &[key, value] : j.items()) {
4445
detail::check_string_size(key.size(), path + "/<key>");
45-
obj.emplace(key, json_to_param_value(value, path + "/" + key, budget, depth + 1));
46+
obj.emplace(key, json_to_param_value(value, std::format("{}/{}", path, key), budget, depth + 1));
4647
}
4748
return obj;
4849
} else {
@@ -83,7 +84,7 @@ TransformSpec parse_transform(const json &j, const std::string &base_path, detai
8384
throw std::runtime_error("JSON parse error at " + path + "/params: Expected object");
8485
}
8586
for (auto &[key, value] : j["params"].items()) {
86-
std::string param_path = path + "/params/" + key;
87+
std::string param_path = std::format("{}/params/{}", path, key);
8788
spec.params[key] = json_to_param_value(value, param_path, budget);
8889
}
8990
}
@@ -154,7 +155,7 @@ ModelSpec parse_model(const json &j, const std::string &base_path, size_t index,
154155
throw std::runtime_error("JSON parse error at " + path + "/params: Expected object");
155156
}
156157
for (auto &[key, value] : j["params"].items()) {
157-
std::string param_path = path + "/params/" + key;
158+
std::string param_path = std::format("{}/params/{}", path, key);
158159
spec.params[key] = json_to_param_value(value, param_path, budget);
159160
}
160161
}
@@ -200,7 +201,7 @@ RuleSpec parse_rule(const json &j, const std::string &base_path, size_t index) {
200201
throw std::runtime_error("JSON parse error at " + action_path + "/args: Expected object");
201202
}
202203
for (auto &[key, value] : action_json["args"].items()) {
203-
std::string arg_path = action_path + "/args/" + key;
204+
std::string arg_path = std::format("{}/args/{}", action_path, key);
204205
action.args[key] = json_to_variant(value, arg_path);
205206
}
206207
}

‎src/loaders/yaml_loader.cpp‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77
#include <cerrno>
88
#include <cstdlib>
9+
#include <format>
910
#include <fstream>
1011
#include <limits>
1112
#include <sstream>
@@ -60,7 +61,7 @@ ParamValue yaml_to_param_value(const YAML::Node &node, const std::string &path,
6061
budget.consume_node(path);
6162

6263
if (node.IsScalar()) {
63-
const std::string scalar = node.as<std::string>();
64+
std::string scalar = node.as<std::string>();
6465
detail::check_string_size(scalar.size(), path);
6566
if (scalar == "true") {
6667
return true;
@@ -87,7 +88,7 @@ ParamValue yaml_to_param_value(const YAML::Node &node, const std::string &path,
8788
ParamArray arr;
8889
arr.reserve(node.size());
8990
for (size_t i = 0; i < node.size(); ++i) {
90-
arr.push_back(yaml_to_param_value(node[i], path + "/" + std::to_string(i), budget, depth + 1));
91+
arr.push_back(yaml_to_param_value(node[i], std::format("{}/{}", path, i), budget, depth + 1));
9192
}
9293
return arr;
9394
}
@@ -98,11 +99,11 @@ ParamValue yaml_to_param_value(const YAML::Node &node, const std::string &path,
9899
for (auto it = node.begin(); it != node.end(); ++it) {
99100
const std::string key = it->first.as<std::string>();
100101
detail::check_string_size(key.size(), path + "/<key>");
101-
obj.emplace(key, yaml_to_param_value(it->second, path + "/" + key, budget, depth + 1));
102+
obj.emplace(key, yaml_to_param_value(it->second, std::format("{}/{}", path, key), budget, depth + 1));
102103
}
103104
return obj;
104105
} else {
105-
throw std::runtime_error("YAML parse error at " + path + ": Expected scalar/sequence/map value");
106+
throw std::runtime_error(std::format("YAML parse error at {}: Expected scalar/sequence/map value", path));
106107
}
107108
}
108109

@@ -149,7 +150,7 @@ TransformSpec parse_transform(const YAML::Node &node, const std::string &base_pa
149150
}
150151
for (auto it = node["params"].begin(); it != node["params"].end(); ++it) {
151152
std::string key = it->first.as<std::string>();
152-
std::string param_path = path + "/params/" + key;
153+
std::string param_path = std::format("{}/params/{}", path, key);
153154
spec.params[key] = yaml_to_param_value(it->second, param_path, budget);
154155
}
155156
}
@@ -219,7 +220,7 @@ ModelSpec parse_model(const YAML::Node &node, size_t index, detail::ParamParseBu
219220
}
220221
for (auto it = node["params"].begin(); it != node["params"].end(); ++it) {
221222
std::string key = it->first.as<std::string>();
222-
std::string param_path = path + "/params/" + key;
223+
std::string param_path = std::format("{}/params/{}", path, key);
223224
spec.params[key] = yaml_to_param_value(it->second, param_path, budget);
224225
}
225226
}
@@ -265,7 +266,7 @@ RuleSpec parse_rule(const YAML::Node &node, size_t index) {
265266
}
266267
for (auto it = action_node["args"].begin(); it != action_node["args"].end(); ++it) {
267268
std::string key = it->first.as<std::string>();
268-
std::string arg_path = action_path + "/args/" + key;
269+
std::string arg_path = std::format("{}/args/{}", action_path, key);
269270
action.args[key] = yaml_to_variant(it->second, arg_path);
270271
}
271272
}

‎src/model/state_space_siso_discrete.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,8 @@ std::vector<double> StateSpaceSisoDiscreteModel::flatten_square_matrix(const std
2828
return flat;
2929
}
3030

31-
StateSpaceSisoDiscreteModel::StateSpaceSisoDiscreteModel(const std::string &id, std::vector<std::vector<double>> a_d,
31+
StateSpaceSisoDiscreteModel::StateSpaceSisoDiscreteModel(const std::string &id,
32+
const std::vector<std::vector<double>> &a_d,
3233
std::vector<double> b_d, std::vector<double> c, double d,
3334
std::vector<double> x0, const std::string &output_signal_path,
3435
const std::string &input_signal_path, SignalNamespace &ns)

‎tests/.clang-tidy‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
# Test code has different concerns than production code. Inherit the repo's
3+
# checks but relax the ones that are pure noise in tests:
4+
# - performance-*: tests are not performance-sensitive; reserve/move/copy
5+
# micro-optimizations add nothing to throwaway fixtures.
6+
# - bugprone-narrowing-conversions: intentional narrowing of small integer
7+
# literals in test arithmetic.
8+
# - bugprone-unchecked-optional-access: false-positive against the gtest
9+
# ASSERT_TRUE(opt.has_value()) guard idiom, which clang-tidy's optional
10+
# model does not treat as a precondition.
11+
# - bugprone-reserved-identifier: test-local identifier conventions.
12+
InheritParentConfig: true
13+
Checks: >
14+
-performance-*,
15+
-bugprone-narrowing-conversions,
16+
-bugprone-unchecked-optional-access,
17+
-bugprone-reserved-identifier
18+
...

‎tests/unit/namespace_test.cpp‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,9 @@
22

33
#include <gtest/gtest.h>
44

5+
#include <algorithm>
6+
#include <string>
7+
58
using namespace fluxgraph;
69

710
class SignalNamespaceTest : public ::testing::Test {

0 commit comments

Comments
 (0)