Repository navigation
fix(clang-tidy): exclude external Bazel repositories from findings - #995
Conversation
| Label("//:.clang-tidy"), | ||
| ], | ||
| lint_target_headers = True, | ||
| header_filter = "^(?!.*/(third_party|external)/).*$", |
There was a problem hiding this comment.
I have the fear that this filter is too broad. Can we at least specify that third_party and external have to be at the start of the path?
| header_filter = "^(?!.*/(third_party|external)/).*$", | |
| header_filter = "^(?!(third_party|external)/).*$", |
There was a problem hiding this comment.
Good catch, applied — anchored both regexes to the start of the path in 4b34c40. Verified this doesn't reopen the leak: Bazel resolves _virtual_includes header symlinks to their real location before clang-tidy sees them, so external headers are always reported with a path starting directly with external/<repo>+/... (never with a bazel-out/.../bin/ prefix in front). Diffed clang-tidy's suppressed-warning counts for both regex forms across score/mw/com/... — byte-for-byte identical output, 0 external/third_party leaks either way. Own-repo findings are unaffected.
| clang-analyzer-*, | ||
|
|
||
| HeaderFilterRegex: '^(?!.*/third_party/).*' | ||
| HeaderFilterRegex: '^(?!.*/(third_party|external)/).*' |
There was a problem hiding this comment.
Same
| HeaderFilterRegex: '^(?!.*/(third_party|external)/).*' | |
| HeaderFilterRegex: '^(?!(third_party|external)/).*' |
There was a problem hiding this comment.
Applied, same reasoning as the other comment — see 4b34c40.
…se-score#1000) ## Summary Fixes all `misc-use-anonymous-namespace` clang-tidy findings (74 findings across 24 files) by manually applying the transformation the check recommends, since **clang-tidy does not implement an automatic FixIt for this check**. ## Why this couldn't be auto-fixed by the pipeline I initially assumed the empty `--fix` patches produced by `aspect_rules_lint` were a pipeline bug and spent time debugging it (verbose clang-tidy invocation, manual sandbox reproduction, etc.). Root cause: this is **not a pipeline issue** — `misc-use-anonymous-namespace` only emits a plain warning/note; it never attaches a `FixItHint`, confirmed by: - Manually re-running the exact recorded clang-tidy `--fix` command (extracted from the actual Bazel sandboxed action) outside Bazel: exit 0, findings printed, but the file is byte-for-byte identical before/after. - clang-tidy's own docs / source for this check. So the fixes below were applied by hand, following exactly what the check's message recommends ("move to anonymous namespace instead"). ## Changes - **22 tutorial/doc files** (`score/mw/com/doc/tutorial/chapter_{1,2,3,4,5,6,7,8,9,10,12}/{consumer,provider}`): wrapped file-scope `static` globals/functions (e.g. `g_running`, `g_shutdown_mutex`, `SignalHandler`, `SendSample`, `UpdateField`, ...) in an unnamed namespace and dropped the now-redundant `static` keyword. - **`score/mw/com/impl/rust/com-api/com-api-ffi-lola/registry_bridge_macro.h`**: the `BEGIN_EXPORT_MW_COM_INTERFACE` / `EXPORT_MW_COM_EVENT` macros generate `static ..._reg_instance;` declarations, which were the source of the remaining findings in `vehicle_gen.cpp` and `bigdata_com_api_gen.cpp` (the only two call sites of these macros in the repo). Fixed at the single root cause (the macro definitions) by wrapping the generated declarations in a nested anonymous namespace instead of touching the two generated call-site files. ## Verification ``` bazel build --config=clang-tidy //score/mw/com/doc/tutorial/... //score/mw/com/example/com-api-example/... //score/mw/com/test/basic_rust_api/... ``` → 0 remaining `misc-use-anonymous-namespace` findings in the affected scope. ``` bazel build //score/mw/com/doc/tutorial/... //score/mw/com/example/com-api-example/... //score/mw/com/test/basic_rust_api/... ``` → builds cleanly, including the Rust FFI bridge consumers of the modified macro. ## Scope note Per the earlier discussion, this PR is scoped **only** to `misc-use-anonymous-namespace` and does not touch the `readability-magic-numbers` (eclipse-score#993), external-repo header-filter (eclipse-score#995), or magic-numbers-check-disable (eclipse-score#999) changes from the other PRs.
…ss (modernize-use-equals-delete) (eclipse-score#1001) ## Summary Fixes all 204 `modernize-use-equals-delete` clang-tidy findings, all of which report "deleted member function should be public". ## Root cause This category of finding has **no available FixIt** in clang-tidy: the check's autofix only adds `= delete` to a non-deleted function — it never relocates a declaration across access specifiers. Every flagged function here was already `= delete`, just placed under `protected`/`private` instead of `public`, so a manual fix was required (same category of limitation previously found for `misc-use-anonymous-namespace` in eclipse-score#1000). ## Scope 204 raw findings across 41 `.cpp` translation units collapse to **8 header files** where the flagged declarations actually live: - `score/message_passing/i_client_connection.h` - `score/message_passing/i_client_factory.h` - `score/message_passing/i_server.h` - `score/message_passing/i_server_connection.h` - `score/message_passing/i_server_factory.h` - `score/message_passing/i_shared_resource_engine.h` - `score/mw/com/impl/proxy_base.h` - `score/mw/com/impl/skeleton_event_base.h` ## Fix Moved the deleted copy/move constructor and assignment operator declarations (with their doc comments) into each class's `public:` section, right after the destructor. Non-deleted (defaulted) constructors/destructors stay in their original `protected:` section. For `proxy_base.h` and `skeleton_event_base.h`, only the deleted **copy** members were relocated — the defaulted **move** members correctly remain protected (unflagged, unchanged). ## Verification - `bazel build --config=clang-tidy` across `score/mw/com/...`, `score/message_passing/...`, `score/memory/...`, `quality/integration_testing/...` → **0 remaining `modernize-use-equals-delete` findings**, no new findings introduced. - Plain `bazel build` over the same scope → compiles cleanly, no regressions. This is a standalone PR per the maintainers' preference to keep each distinct clang-tidy concern isolated (see eclipse-score#995, eclipse-score#999, eclipse-score#1000 for prior examples in this series).
35e8d0e to
4b34c40
Compare
clang-tidy findings currently include diagnostics reported in headers
from external Bazel repositories (e.g. bazel-out/.../external/
score_baselibs+/...), which we cannot fix or influence upstream.
Root cause: the clang_tidy aspect was configured with
lint_target_headers = True, which makes aspect_rules_lint compute its
own -header-filter from the target's own header directories instead of
honoring //:.clang-tidy's HeaderFilterRegex. That computed filter falls
back to matching every header (".*") whenever a target has more than
one header directory (e.g. due to _virtual_includes), so findings from
external/ and third_party/ leaked through unfiltered.
Fix: disable lint_target_headers and pass an explicit header_filter
matching //:.clang-tidy's HeaderFilterRegex, which now also excludes
external/ in addition to third_party/.
HeaderFilterRegex anchors third_party/ and external/ to the start of
the path (rather than allowing them anywhere via a leading '.*/'), per
review feedback, to avoid accidentally excluding legitimate own-repo
paths that merely contain 'third_party' or 'external' as a path
segment further down (e.g. a hypothetical score/.../external/foo.h).
Verified this is not overly narrow: Bazel resolves _virtual_includes
header symlinks to their real location before clang-tidy sees them, so
external headers are always reported with a path starting directly
with 'external/<repo>+/...' (never with a 'bazel-out/.../bin/' prefix
in front of it) -- confirmed empirically by diffing suppressed-warning
counts for both regex forms across score/mw/com/..., which are
byte-for-byte identical.
Verification:
- Built //score/mw/com/... with --config=clang-tidy and aggregated all
generated SARIF reports: 0 external/third_party findings leak
through with the anchored regex, identical to the previous
non-anchored one.
- Own-repo findings (in .cpp and .h files) are still correctly
reported, unaffected by the tightened regex.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ndings (eclipse-score#1012) ## Summary Fixes all `bugprone-suspicious-stringview-data-usage` clang-tidy findings in the codebase. ## Root cause All findings occur at call sites of `score::json::Object::find()`. `score::json::Object` is a `std::map<StringComparisonAdaptor, Any>`, and `StringComparisonAdaptor` has an implicit constructor from `std::string_view`. Despite this, many call sites narrowed a `std::string_view` key down to a raw `const char*` via `.data()` before calling `find()`: ```cpp constexpr auto kHvSocketConfigurationKey = "hypervisor-socket"sv; ... const auto socket_entry = obj.find(kHvSocketConfigurationKey.data()); ``` `bugprone-suspicious-stringview-data-usage` flags this pattern because `.data()` discards the view's length and the result is not guaranteed to be null-terminated in general — passing the raw `char*` on to an API that expects a null-terminated C-string is a latent bug, even though in these specific cases the views happen to originate from null-terminated string literals. ## Fix Pass the `std::string_view` key directly to `find()` instead of calling `.data()` on it: ```cpp const auto socket_entry = obj.find(kHvSocketConfigurationKey); ``` This relies on the existing implicit `StringComparisonAdaptor(const std::string_view&)` conversion, removes the unsafe pattern, and is a pure call-site simplification with no behavioral change (same key comparison semantics, just without the intermediate C-string narrowing). ## Files changed - `score/mw/com/gateway/transport_layer/sample/configuration/sample_transport_config_parser.cpp` - `score/mw/com/gateway/gateway_application/configuration/gateway_config_parser.cpp` - `score/mw/com/impl/configuration/configuration_json_parsing_strategy.cpp` 53 call sites fixed across the three files (5 + 5 + 43 respectively). ## Verification - Full `bazel build //score/mw/com/... //score/memory/... //score/message_passing/...` passes. - Scoped clang-tidy build isolated to `bugprone-suspicious-stringview-data-usage` shows **0 findings** (down from 53). - All 51 tests under `score/mw/com/impl/configuration/...` and `score/mw/com/gateway/...` pass. ## Scope This PR intentionally contains **only** `bugprone-suspicious-stringview-data-usage` fixes, per the project convention of one clang-tidy check per PR (see eclipse-score#995, eclipse-score#999, eclipse-score#1000, eclipse-score#1001, eclipse-score#1003).
…clipse-score#1014) ## Summary Fixes all `cppcoreguidelines-init-variables` clang-tidy findings (11 findings across 4 files). ## Approach Initializes each flagged variable at its point of declaration instead of leaving it default-constructed/uninitialized, per the C++ Core Guidelines' recommendation that every variable be given a well-defined value as soon as it is declared. All changes are pure initialization additions — no behavioral change, since every affected variable is fully overwritten before its first read in the original code, and the chosen initial values (enum default, `0`, `false`) match the variables' subsequent overwritten semantics. ## Fixes - **`unix_domain_engine.cpp`**: `PipeEvent pipe_event` is an out-parameter for a raw `read()` syscall that always overwrites it before use; initialized to `PipeEvent::QUIT` (a valid enumerator) as a safe default. - **`unix_domain_server.cpp`**: `std::uint8_t code` is an out-parameter for `ReceiveProtocolMessage()`; zero-initialized. - **`flock_test.cpp`**: `FILE* stream` was declared then immediately assigned from `fdopen()` on the next line; merged declaration and initialization (×2 occurrences). `int c` (loop-local `fgetc()` result) is zero-initialized. - **`smokeyeyes.cpp`**: the `boost::program_options`-bound variables (`num_clients`, `turns`, `batch_size`, `no_wait`, `num_slots`) are populated by `po::store`/`po::notify` from their `default_value()`s; initialized to matching defaults so the variable is never observably uninitialized between declaration and parsing. `int wstatus`, an out-parameter for `waitpid()`, is zero-initialized. ## Verification - Scoped clang-tidy build isolated to `cppcoreguidelines-init-variables` shows **0 findings** (down from 11 across 4 files). - Full `bazel build //score/message_passing/... //score/mw/com/test/flock/... //score/mw/com/test/smokeyeyes/...` passes. - `bazel test //score/message_passing:unix_domain_test //score/message_passing:unix_domain_test_linux` passes (2/2). - `bazel test //:format_test_C++_with_clang-format` passes — no formatting issues introduced. ## Scope This PR intentionally contains **only** `cppcoreguidelines-init-variables` fixes, per the project convention of one clang-tidy check per PR (see eclipse-score#995, eclipse-score#999, eclipse-score#1000, eclipse-score#1001, eclipse-score#1003, eclipse-score#1012, eclipse-score#1013).
…indings (#1015) ## Summary Fixes all `cppcoreguidelines-special-member-functions` clang-tidy findings (3 findings across 3 files). ## Approach Explicitly declares the missing copy/move special member functions (deleted, since none of these classes support meaningful copy/move semantics) so each class fully follows the Rule of Five instead of relying on implicit generation, per the C++ Core Guidelines' recommendation that if any of destructor/copy/move is user-declared, all five should be declared. ## Fixes - **`sample_sender_receiver.cpp`**: `TestDestructor` is a scope-guard type that calls `stop_source_.request_stop()` in its destructor; it is only ever used as a named local variable and was never intended to be copied or moved. Deleted copy/move constructor and assignment. - **`provider.cpp` (partial_restart/provider_restart)**: `CyclicEventSender` already explicitly deleted its copy constructor/assignment (its `std::thread` member isn't copyable) but didn't declare move operations. Since its background thread captures `this`, moving the object would leave the thread referencing a stale instance, so move is deleted too, for the same reason copy already was. - **`smokeyeyes.cpp`**: `SharedState` owns a `pthread_barrier_t` that is placed directly into shared memory via placement-new and destroyed in its destructor; the barrier is tied to this object's address, so copying or moving it is not meaningful. Deleted copy/move constructor and assignment. All changes are pure declarations (`= delete`) with no behavioral change, since none of these types were ever copied or moved elsewhere in the codebase. ## Verification - Scoped clang-tidy build isolated to `cppcoreguidelines-special-member-functions` shows **0 findings** (down from 3 across 3 files). - Full `bazel build //score/mw/com/test/common_test_resources/... //score/mw/com/test/partial_restart/... //score/mw/com/test/smokeyeyes/...` passes. - `bazel test //score/mw/com/test/partial_restart/provider_restart/integration_test:provider_restart_graceful` passes. - `bazel test //:format_test_C++_with_clang-format` passes. ## Scope This PR intentionally contains **only** `cppcoreguidelines-special-member-functions` fixes, per the project convention of one clang-tidy check per PR (see #995, #999, #1000, #1001, #1003, #1012, #1013, #1014).
…clipse-score#1003) ## Summary Fixes all in-repo findings of the `performance-unnecessary-value-param` clang-tidy check, which flags function parameters passed by value but only ever read (never moved-from), recommending `const&` instead to avoid an unnecessary copy. ## Why this needed extensive manual work Unlike some other checks fixed in earlier PRs, this check's FixIt is **partially** automatic and has two distinct incompleteness modes: 1. **Declaration/definition split**: the FixIt only patches the diagnostic location (usually the out-of-line `.cpp` definition), never the paired header declaration. For member functions this causes a hard compile error (signature mismatch); for free functions it can silently create a duplicate overload, surfacing as a linker error instead. 2. **No FixIt at all** for header-only inline functions, template member functions, and constructors with member-initializer lists — confirmed by inspecting the generated patch files (many were empty even at flagged locations). This PR: - Applies the upstream FixIt patches to affected `.cpp` files. - Manually synchronizes every affected header declaration to match. - Manually converts ~25 header-only inline functions, constructors, and templates that clang-tidy flags but cannot autofix. - Removes now-redundant `std::move()` calls at call sites where the callee parameter changed from by-value to `const&`, including cascading cases uncovered once an inner callee was fixed (e.g. `ServiceDiscovery::BindingSpecificStartFindService` → `StartFindServiceImpl`, and `ServiceElementMessage`'s derived `Register/UnregisterNotificationRequest`, `UpdateNotification`). - Updates `score/mw/com/api_surface.lock.json` to reflect the intentional public signature change of `Runtime::ResolveInstanceIDs`. ## Excluding `std::shared_ptr` and `std::weak_ptr` from this check Added `std::shared_ptr;std::weak_ptr` to `performance-unnecessary-value-param.AllowedTypes` (note: the option matches on the type name *without* template arguments, so the value is `std::shared_ptr`/`std::weak_ptr`, not `std::shared_ptr<.*>`/`std::weak_ptr<.*>`) and reverted every `std::shared_ptr`- and `std::weak_ptr`-parameter change originally made for this check back to its original by-value form. Rationale: copying a `shared_ptr`/`weak_ptr` is a cheap atomic refcount operation, not a heap allocation, and this check has no interprocedural view of pass-through call chains — it was flagging intermediate forwarding functions individually even though only a single copy ultimately occurs at whichever function actually owns/stores the pointer. Rather than fight the checker's local view on every future shared_ptr/weak_ptr parameter, both are now excluded from this check entirely. ## Applying the C++ Core Guidelines "consume parameter" pattern (F.18) While auditing every `const&` conversion in this PR that removed a pre-existing `std::move()`, one genuine sink/consume parameter was found: `GatewayApplication`'s constructors used to take `GatewayConfiguration` by value and `std::move` it into the `app_configuration_` member. Naively converting that parameter to `const&` (as clang-tidy suggests) would silently turn the move into a copy. Per [F.18](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rf-consume) ("for 'will-move-from' parameters, pass by `X&&` and `std::move` the parameter"), this parameter is instead converted to `GatewayConfiguration&&`, preserving the move into the member. All call sites already pass a prvalue (`MakeConfig(...)`), so no call-site changes were required. Every other `const&` conversion in this PR that also removed a `std::move()` was audited and found **not** to be a genuine sink case — in each instance the `std::move()` was feeding a parameter that was itself already `const&` (e.g. `Skeleton::Create`, `ServiceElementMessage`'s base constructor, `std::promise::set_value`), so the move was already a no-op before this PR and removing it causes no behavioral change. ## Keeping `PolymorphicOffsetPtrAllocator::deallocate` pass-by-value Reverted `deallocate`'s pointer parameter back to by-value with a `NOLINT` + justification comment, instead of converting it to `const&`. The C++ standard's `Cpp17Allocator` named requirements (`[allocator.requirements]`) specify the expression `a.deallocate(p, n)` matching `std::allocator::deallocate`'s by-value signature, which `std::allocator_traits` and allocator-aware containers rely on. Deviating from that standard interface shape for a small, trivially-copyable pointer-like type (`OffsetPtr<T>`) has no measurable benefit. ## Keeping `PolymorphicOffsetPtrAllocator::deallocate` pass-by-value Reverted `deallocate`'s pointer parameter back to by-value with a `NOLINT` + justification comment, instead of converting it to `const&`. The C++ standard's `Cpp17Allocator` named requirements (`[allocator.requirements]`) specify the expression `a.deallocate(p, n)` matching `std::allocator::deallocate`'s by-value signature, which `std::allocator_traits` and allocator-aware containers rely on. Deviating from that standard interface shape for a small, trivially-copyable pointer-like type (`OffsetPtr<T>`) has no measurable benefit. ## Fixing `modernize-pass-by-value` findings triggered by this check's own fixes Some of this check's `const&` conversions newly triggered `modernize-pass-by-value`: `ServiceTypeDeployment`, `ServiceInstanceDeployment`, and `ConfigurationStore`'s constructors take a parameter by `const&` and then unconditionally copy it into a member — exactly the "pass by value and `std::move`" pattern that check flags. These constructors were converted to take the affected parameters (`BindingInformation`, `ServiceIdentifierType`, `LolaServiceTypeDeployment`) by value with `std::move` into the member, satisfying both checks simultaneously. All other in-repo `modernize-pass-by-value` findings were confirmed (via aggregating `bazel build --config=clang-tidy` SARIF reports) to be pre-existing on `upstream/main` and unrelated to this PR, so are intentionally left untouched. ## Out of scope Six remaining findings in `external/score_baselibs+` headers (`dynamic_array.h`, `non_relocatable_vector.h`) are in a vendored external dependency outside this repo's control, consistent with the precedent established in the exclude-external-findings PR (eclipse-score#995). ## Verification - Full plain build: succeeds, no compile/link errors. - Full `--config=clang-tidy` build (scoped to this check, plus `modernize-pass-by-value`): 0 remaining in-repo findings for either check (only the 6 external + 6 pre-existing/unrelated findings noted above remain), confirmed after the `std::shared_ptr`/`std::weak_ptr` exclusion/revert, the F.18 conversion, and the `modernize-pass-by-value` fixes. - Full test suite: passes, excluding pre-existing Docker network-pool-exhaustion environment failures (`all predefined address pools have been fully subnetted`) unrelated to this change.
Summary
clang-tidy findings currently include diagnostics reported in headers from
external Bazel repositories (e.g.
bazel-out/.../external/score_baselibs+/...),which we cannot fix or influence upstream. This excludes them from results.
Root cause
The clang-tidy aspect (
tools/lint/linters.bzl) was configured withlint_target_headers = True. This makesaspect_rules_lintcompute its own-header-filterregex from the target's own header directories instead ofhonoring
//:.clang-tidy'sHeaderFilterRegex. That computed filter fallsback to matching every header (
.*) whenever a target has more than oneheader directory (e.g. because of
_virtual_includes), so findings fromexternal/andthird_party/leaked through unfiltered.Fix
.clang-tidy:HeaderFilterRegexnow also excludesexternal/inaddition to
third_party/.tools/lint/linters.bzl: disabledlint_target_headersand instead passan explicit
header_filtermatching//:.clang-tidy'sHeaderFilterRegexdirectly to the aspect (relying on clang-tidy's own config-file discovery
from inside the Bazel sandbox proved unreliable).
Verification
Built
//score/mw/com/...with--config=clang-tidyand aggregated allgenerated SARIF reports:
external/score_baselibs+/...headers..cppfiles, 11 in our own.hheaders, e.g.score/mw/com/impl/bindings/lola/*.h) are still correctly reported.