Skip to content

Commit 5e5dcc9

Browse files
committed
Triage the first clang-tidy run
Disable three checks that fire systemically on framework-dictated or deliberate patterns, documented in .clang-tidy: - performance-unnecessary-value-param: every NIF takes its ResourcePtr args by value per Fine's FINE_NIF decode convention (the cppcheck build suppresses the same thing as passedByValueCallback). - bugprone-throwing-static-initialization: FINE_NIF/FINE_RESOURCE register at static-init via throwing ctors, across macro expansions. - performance-enum-size: Opcode and ref::Kind are int64_t on purpose to pack into the int64 refs the Elixir lowerer emits. Fix the genuine findings: - Rename the reserved-identifier namespaces __async/__op (leading double underscore is reserved) to async_detail/op_detail. - Bind window-reduce shapes by const reference instead of copying (performance-unnecessary-copy-initialization), matching the existing style a few lines up. - Widen `rank` before the `2 * rank` reserve() so the multiplication happens in size_t (bugprone-implicit-widening-of-multiplication-result). Suppress the remaining false positives inline, with rationale: - The make_binary_from_cstr memcpy targets a length-counted BEAM binary, not a C string (bugprone-not-null-terminated-result). - ~Program/~Reaper/~WorkerThread are best-effort cleanup destructors where a throw is already fatal (bugprone-exception-escape); the check stays enabled for every other function.
1 parent 6840885 commit 5e5dcc9

6 files changed

Lines changed: 48 additions & 18 deletions

File tree

.clang-tidy

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,16 +4,30 @@
44
# env) — see lib/mix/tasks/clang.tidy.ex and the `clang-tidy` target in
55
# the Makefile.
66
#
7-
# A focused, high-signal starting set: bug patterns, the clang static
8-
# analyzer (clang-analyzer-*), and performance checks. Deliberately not
9-
# the modernize/readability/cppcoreguidelines firehose yet — broaden once
10-
# this baseline is green. `bugprone-easily-swappable-parameters` is off
11-
# because it fires on nearly every NIF entry point (many same-typed args)
12-
# without pointing at a real defect.
7+
# A focused, high-signal set: bug patterns, the clang static analyzer
8+
# (clang-analyzer-*), and performance checks. Deliberately not the
9+
# modernize/readability/cppcoreguidelines firehose yet — broaden once this
10+
# baseline stays green.
11+
#
12+
# Disabled checks and why:
13+
# * bugprone-easily-swappable-parameters — fires on nearly every NIF
14+
# entry point (many same-typed args) without pointing at a real defect.
15+
# * performance-unnecessary-value-param — every NIF takes its
16+
# fine::ResourcePtr args by value because Fine's FINE_NIF macro decodes
17+
# each BEAM term into a value and passes it in; the signature is the
18+
# binding convention, not a stray copy (the cppcheck build suppresses
19+
# the same thing as passedByValueCallback).
20+
# * bugprone-throwing-static-initialization — FINE_NIF / FINE_RESOURCE
21+
# register callbacks at static-init time via throwing constructors;
22+
# this is inherent to Fine's registration model, across ~25 macro
23+
# expansions we don't own.
24+
# * performance-enum-size — Opcode and ref::Kind are int64_t on purpose
25+
# (they pack into the int64 refs the Elixir lowerer emits); shrinking
26+
# the base type would break that ABI.
1327
#
1428
# HeaderFilterRegex scopes diagnostics to our own headers; MLX and Fine
1529
# arrive via -isystem and are skipped, exactly as the compiler skips them.
16-
Checks: '-*,bugprone-*,clang-analyzer-*,performance-*,-bugprone-easily-swappable-parameters'
30+
Checks: '-*,bugprone-*,clang-analyzer-*,performance-*,-bugprone-easily-swappable-parameters,-performance-unnecessary-value-param,-bugprone-throwing-static-initialization,-performance-enum-size'
1731
WarningsAsErrors: '*'
1832
HeaderFilterRegex: 'c_src/'
1933
FormatStyle: none

c_src/emily/async.hpp

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,16 @@ namespace emily {
4444

4545
namespace mx = mlx::core;
4646

47-
namespace __async {
47+
namespace async_detail {
4848

4949
// Build a binary term in msg_env from a null-terminated C string.
5050
inline ERL_NIF_TERM make_binary_from_cstr(ErlNifEnv *msg_env, const char *s) {
5151
size_t len = std::strlen(s);
5252
ERL_NIF_TERM term;
5353
unsigned char *data = enif_make_new_binary(msg_env, len, &term);
54+
// The destination is a length-counted BEAM binary of exactly `len`
55+
// bytes, not a C string, so a trailing NUL is neither needed nor wanted.
56+
// NOLINTNEXTLINE(bugprone-not-null-terminated-result)
5457
std::memcpy(data, s, len);
5558
return term;
5659
}
@@ -80,7 +83,7 @@ error_reason_from_current_exception(ErlNifEnv *msg_env) {
8083
}
8184
}
8285

83-
} // namespace __async
86+
} // namespace async_detail
8487

8588
// Run `build_payload` on the worker thread of `w` and post the
8689
// result back to the caller PID as a message. Returns a fresh ref
@@ -132,7 +135,7 @@ fine::Term async_reply(ErlNifEnv *env,
132135
msg_env, ref_in_msg,
133136
enif_make_tuple2(
134137
msg_env, fine::encode(msg_env, emily::atoms::error),
135-
__async::error_reason_from_current_exception(msg_env)));
138+
async_detail::error_reason_from_current_exception(msg_env)));
136139
}
137140
}
138141

c_src/emily/op_cores.hpp

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -154,8 +154,8 @@ inline mx::array sliding_windows_view(
154154
out_dims.assign(rank, 0);
155155
mx::Shape new_shape;
156156
mx::Strides new_strides;
157-
new_shape.reserve(2 * rank);
158-
new_strides.reserve(2 * rank);
157+
new_shape.reserve(2 * static_cast<std::size_t>(rank));
158+
new_strides.reserve(2 * static_cast<std::size_t>(rank));
159159

160160
for (int i = 0; i < rank; ++i) {
161161
int64_t eff = (window_shape[i] - 1) * dilations[i] + 1;
@@ -251,11 +251,11 @@ inline mx::array window_scatter_core(
251251
bool is_max,
252252
mx::Stream &s) {
253253
int rank = static_cast<int>(window_shape.size());
254-
auto original_shape = tensor.shape();
254+
const auto &original_shape = tensor.shape();
255255

256256
// 1. Pad input with init_value.
257257
auto padded = do_pad(tensor, pad_lo, pad_hi, init_value, s);
258-
auto padded_shape = padded.shape();
258+
const auto &padded_shape = padded.shape();
259259

260260
// 2. Sliding-window view (dilation is implicitly 1 per axis for scatter).
261261
std::vector<int64_t> dilations(rank, 1);
@@ -329,7 +329,7 @@ inline mx::array window_scatter_core(
329329

330330
// 7. Reshape source so each index tuple is a single-point write.
331331
mx::Shape source_reshape;
332-
source_reshape.reserve(2 * rank);
332+
source_reshape.reserve(2 * static_cast<std::size_t>(rank));
333333
for (int i = 0; i < rank; ++i)
334334
source_reshape.push_back(static_cast<mx::ShapeElem>(out_dims[i]));
335335
for (int i = 0; i < rank; ++i)

c_src/emily/opcodes.hpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -280,7 +280,7 @@ inline double f64_from_bits(int64_t bits) {
280280
return d;
281281
}
282282

283-
namespace __op {
283+
namespace op_detail {
284284

285285
inline const mx::array &arg1(const std::vector<mx::array> &in,
286286
const char *name) {
@@ -338,13 +338,13 @@ inline bool keepdims_attr(const std::vector<std::vector<int64_t>> &a,
338338
return v[0] != 0;
339339
}
340340

341-
} // namespace __op
341+
} // namespace op_detail
342342

343343
// Replay one instruction: apply `op` to its resolved operands + attrs.
344344
inline mx::array dispatch_op(Opcode op, const std::vector<mx::array> &in,
345345
const std::vector<std::vector<int64_t>> &iattrs,
346346
mx::Stream &s) {
347-
using namespace emily::__op;
347+
using namespace emily::op_detail;
348348
switch (op) {
349349
// --- Binary arithmetic / bitwise ---
350350
case Opcode::Add:

c_src/emily/program.hpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,11 @@ class Program {
122122
// * the recycled-`fun_id` collision the on-worker erase guards against
123123
// can't fire on a stopping worker — it runs no further compiles.
124124
// So on the declined path we simply let `drop` destruct below (issue #172).
125+
//
126+
// post_to_worker constructs a std::function (may throw bad_alloc); a throw
127+
// from this best-effort cleanup destructor is unrecoverable and would
128+
// std::terminate regardless, so the escape is accepted here.
129+
// NOLINTNEXTLINE(bugprone-exception-escape)
125130
~Program() {
126131
for (auto &kv : compiled) {
127132
CompiledEntry &entry = kv.second;

c_src/emily/worker.hpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,10 @@ class Reaper {
148148
}
149149
}
150150

151+
// shutdown() joins the reaper thread; std::thread::join can in theory
152+
// throw std::system_error, but a throw from this best-effort teardown
153+
// destructor is unrecoverable and would std::terminate regardless.
154+
// NOLINTNEXTLINE(bugprone-exception-escape)
151155
~Reaper() { shutdown(); }
152156

153157
private:
@@ -198,6 +202,10 @@ class WorkerThread {
198202

199203
// Non-blocking: signal stop and hand the thread to the Reaper to join
200204
// off-scheduler. Pending tasks are cancelled with {:error, :stopped}.
205+
//
206+
// retire() takes a lock and moves the thread to the reaper; a throw here
207+
// is unrecoverable from a destructor and would std::terminate regardless.
208+
// NOLINTNEXTLINE(bugprone-exception-escape)
201209
~WorkerThread() { Reaper::instance().retire(state_.get()); }
202210

203211
// Enqueue a task. Throws if the worker has been stopped or the queue is

0 commit comments

Comments
 (0)