[ML] Fix compiler warnings across the codebase by edsavage · Pull Request #2985 · elastic/ml-cpp

edsavage · 2026-03-12T21:11:14Z

Summary

Reduces Clang warnings from ~2,500 to 86 (all remaining are -Wunsafe-buffer-usage, which require a std::span migration and are out of scope here).
Adds three warning suppressions to clang.cmake:
- -Wno-switch-default (174 warnings) — conflicts with the more useful -Wswitch-enum; adding default: to exhaustive enum switches would suppress missing-case detection when new enum values are added.
- -Wno-nrvo (19 warnings) — purely informational C++23 diagnostic about NRVO eligibility.
- -Wno-missing-noreturn (5 warnings) — all remaining cases are lambdas where [[noreturn]] cannot be applied pre-C++23.
Fixes 80 code warnings across 59 files:
- Removes 37 unused const variables (dead code from state serialisation refactors).
- Removes 2 unused functions (calibrationExperiment, dataGenerator in tests) and 2 unused-but-set variables.
- Fixes 9 shadow warnings by renaming inner variables.
- Fixes 8 implicit int-to-float conversions with explicit static_cast<double>().
- Fixes 2 tautological-compare logic bugs where !(p >= 0.0 || p <= 1.0) was always false — corrected to !(p >= 0.0 && p <= 1.0).
- Removes 2 redundant default: cases in exhaustive enum switches.
- Fixes 1 pessimizing-move, 1 range-loop-bind-reference, 1 sign-compare, 1 shorten-64-to-32, 1 CTAD issue.
- Removes unnecessary virtual from method in final class.
- Adds [[noreturn]] to named function throws().

Test plan

CI passes on all platforms (Linux x86_64, Linux aarch64, macOS aarch64, Windows x86_64)
clang-format check passes
Verify no behavioural changes other than the tautological-compare bug fixes

Made with Cursor

Reduce Clang warnings from ~2500 to 86 (all remaining are -Wunsafe-buffer-usage which require a std::span migration). Compiler flag suppressions (clang.cmake): - -Wno-switch-default: conflicts with the more useful -Wswitch-enum - -Wno-nrvo: purely informational C++23 diagnostic - -Wno-missing-noreturn: remaining cases are lambdas where [[noreturn]] cannot be applied pre-C++23 Code fixes across 59 files: - Remove 37 unused const variables (dead code from state serialisation refactors) - Remove 2 unused functions and 2 unused-but-set variables - Fix 9 shadow warnings by renaming inner variables - Fix 8 implicit int-to-float conversions with static_cast - Fix 2 tautological-compare logic bugs where the condition !(p >= 0.0 || p <= 1.0) was always false - Remove 2 redundant default cases in exhaustive enum switches - Fix 1 pessimizing-move, 1 range-loop-bind-reference, 1 sign-compare, 1 shorten-64-to-32, 1 CTAD issue - Remove unnecessary virtual from method in final class - Add [[noreturn]] to named function throws() - Add missing newline at EOF Made-with: Cursor

prodsecmachine · 2026-03-12T22:28:58Z

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Made-with: Cursor

Made-with: Cursor # Conflicts: # bin/pytorch_inference/Main.cc

Change [=] to [this, f] in CConcurrentWrapper to silence -Wdeprecated on GCC 13 and Clang. Made-with: Cursor

Copilot

Pull request overview

This PR reduces Clang warning volume across the ML C++ codebase and associated tests, including a couple of small correctness fixes, by removing unused declarations, tightening types/casts, and adjusting warning configuration.

Changes:

Add Clang warning suppressions in CMake and remove/adjust code patterns that trigger noisy diagnostics.
Remove unused constants/functions/variables and resolve shadowing / conversion warnings in production and test code.
Fix probability-range validation logic in CCategoricalTools (tautological compare) and other small correctness/typing issues.

Reviewed changes

Copilot reviewed 58 out of 60 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
lib/model/unittest/ModelTestHelpers.h	Marks header-local helper functions as `[[maybe_unused]]` to silence unused-function warnings.
lib/model/unittest/CTokenListDataCategorizerTest.cc	Renames lambda parameter to avoid shadowing/warnings.
lib/model/unittest/CModelMemoryTest.cc	Removes unused string constant from test TU.
lib/model/FunctionTypes.cc	Removes unused `EMPTY_FUNCTIONS` declaration.
lib/model/CSearchKey.cc	Removes unused tag constant(s).
lib/model/CResourceMonitor.cc	Removes redundant `default:` in an exhaustive enum switch.
lib/model/CMetricBucketGatherer.cc	Removes unused `EMPTY_DOUBLE_VEC` constant.
lib/model/CEventRateModel.cc	Adds explicit cast for category id passed to `concentration(...)`.
lib/model/CDetectorEqualizer.cc	Renames lambda parameters to avoid shadowing / improve clarity.
lib/model/CDataGatherer.cc	Removes unused `EMPTY_STRING` constant.
lib/model/CAnnotatedProbability.cc	Removes unused persistence tag constant.
lib/maths/time_series/unittest/CCalendarCyclicTestTest.cc	Fixes sign-compare warning by casting loop index in comparison.
lib/maths/time_series/CTimeSeriesDecompositionStateSerialiser.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CTimeSeriesDecompositionDetail.cc	Adds explicit cast to match expected floating-point parameter types.
lib/maths/time_series/CTimeSeriesDecomposition.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CSeasonalComponentAdaptiveBucketing.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CSeasonalComponent.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CDecompositionComponent.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CCalendarCyclicTest.cc	Makes integer→double conversions explicit to silence warnings.
lib/maths/time_series/CCalendarComponentAdaptiveBucketing.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CCalendarComponent.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/time_series/CAdaptiveBucketing.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/unittest/CToolsTest.cc	Makes mixed-type loop bound comparison explicit.
lib/maths/common/unittest/CMultivariateNormalConjugateTest.cc	Removes unused helper functions in tests.
lib/maths/common/CXMeansOnline1d.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CStatisticalTests.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CPriorStateSerialiser.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CPoissonMeanConjugate.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/COneOfNPrior.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CNormalMeanPrecConjugate.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CNaturalBreaksClassifier.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CMultivariateConstantPrior.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CMultinomialConjugate.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CMultimodalPrior.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CModel.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CLogNormalMeanPrecConjugate.cc	Removes unused `EMPTY_STRING` constant.
lib/maths/common/CGammaRateConjugate.cc	Removes unused `UNKNOWN_VALUE_STRING` constant.
lib/maths/common/CCategoricalTools.cc	Fixes tautological probability-range checks (`
lib/maths/analytics/unittest/CDataFrameUtilsTest.cc	Adds explicit cast for map key conversion in test assertions.
lib/maths/analytics/CBoostedTreeLoss.cc	Removes unused tag constant.
lib/maths/analytics/CBoostedTreeFactory.cc	Removes unused tag constant.
lib/core/unittest/CConcurrencyTest.cc	Marks a test helper as `[[noreturn]]` to address missing-noreturn warnings.
lib/core/CStateMachine.cc	Removes unused tag constants.
lib/core/CJsonStateRestoreTraverser.cc	Removes redundant `default:` in switch used for logging token types.
lib/api/unittest/CTestAnomalyJob.cc	Formatting-only newline/line-ending adjustment.
lib/api/unittest/CInferenceModelMetadataTest.cc	Makes mixed integer/floating arithmetic explicit.
lib/api/unittest/CDataFrameAnalyzerTrainingTest.cc	Removes pessimizing move into `stringstream`.
lib/api/unittest/CAnomalyJobTest.cc	Renames test constants and makes template argument explicit for `uniform_int_distribution`.
lib/api/CSingleFieldDataCategorizer.cc	Renames lambda captures to avoid shadowing warnings.
lib/api/CModelSizeStatsJsonWriter.cc	Removes unused JSON field name constant.
lib/api/CForecastRunner.cc	Makes integer→double conversion explicit before applying fractional limit.
lib/api/CFieldDataCategorizer.cc	Renames lambda captures to avoid shadowing warnings in background persistence.
lib/api/CDetectionRulesJsonParser.cc	Removes unused `CONDITION` constant (keeps `CONDITIONS`).
lib/api/CDataFrameAnalysisInstrumentation.cc	Removes unused JSON tag constants.
include/model/CMetricModelFactory.h	Removes redundant `virtual` on method in a `final` class.
include/maths/common/CBootstrapClusterer.h	Removes unused locals and updates trace logging to derive partition sizes from parity vector.
include/maths/common/CBasicStatistics.h	Makes coordinate conversion explicit in custom central-moments add.
include/core/CConcurrentWrapper.h	Makes lambda capture explicit to avoid deprecated implicit `this` capture warnings.
cmake/compiler/clang.cmake	Adds warning suppressions for `missing-noreturn`, `nrvo`, and `switch-default`.
bin/pytorch_inference/Main.cc	Adds explicit cast for Torch thread count API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-22T21:38:02Z

lib/core/CJsonStateRestoreTraverser.cc

        case SBoostJsonHandler::E_TokenStringPart:
            tokenTypeName = "string_part";
            break;
-        default:
-            tokenTypeName = "unknown";
-            break;
        }

        LOG_ERROR(<< "JSON state must be object at root. Found token type: " << tokenTypeName


With the default: removed, tokenTypeName will remain the empty string if m_Handler.s_Type ever has an unexpected value (e.g. new enum value added later), which makes the log line less informative. Consider initializing tokenTypeName to something like "unknown" before the switch (so you can keep the switch exhaustive without reintroducing a default:).

Address Copilot review: if a new enum value is added to SBoostJsonHandler, the switch won't cover it and the log message would contain an empty string. Initialising to "unknown" keeps the error log informative. Made-with: Cursor

Copilot

Pull request overview

Copilot reviewed 58 out of 60 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-07T22:19:48Z

lib/maths/analytics/unittest/CDataFrameUtilsTest.cc

        BOOST_REQUIRE_EQUAL(actualCategoryCounts.size(), expectedCategoryCounts.size());
        for (std::size_t i = 0; i < expectedCategoryCounts.size(); ++i) {
-            BOOST_REQUIRE_EQUAL(actualCategoryCounts[i], expectedCategoryCounts[i]);
+            BOOST_REQUIRE_EQUAL(actualCategoryCounts[i],
+                                expectedCategoryCounts[static_cast<double>(i)]);
        }


expectedCategoryCounts is a boost::unordered_map<double, double>, but this loop iterates i from 0..expectedCategoryCounts.size()-1 and indexes both maps with operator[]. If category IDs are not contiguous (e.g. missing an intermediate category), this will (a) insert new keys with 0.0 via operator[] and (b) fail to compare counts for any category key > size()-1, so the test can silently miss mismatches. Iterate over the actual keys (e.g. for (const auto& [category, expected] : expectedCategoryCounts)) and compare against actualCategoryCounts via find()/at() without mutating either map.

Address Copilot review: the loop assumed contiguous integer category IDs (0..N-1) and used operator[] which silently inserts missing keys. Iterate over actual map entries instead. Made-with: Cursor

- CDataFrameTrainBoostedTreeRunner: add LOG_ABORT after 3 exhaustive switch statements that had no default — falling off the end of a non-void function is undefined behaviour (MSVC C4715, GCC -Wreturn-type) - CBoostedTreeHyperparameters: initialise hyperparameterValue to 0.0 — previously uninitialised if a new enum value was added without updating the switch (MSVC C4701) - CJsonLogLayout: remove dangling reference to temporary from boost::log::extract().get() — binding const auto& to the result of .get() on a temporary extractor leaves the reference dangling after the full expression ends (GCC -Wdangling-reference) Made-with: Cursor

Change [=] to [=, this] in 6 lambdas that capture this. Implicit this capture via [=] is deprecated in C++20. (GCC -Wdeprecated) Made-with: Cursor

Add static_cast<double> at 7 locations where size_t values are implicitly converted to double. (GCC -Wconversion, MSVC C4244/C4267) Made-with: Cursor

edsavage · 2026-04-08T01:55:30Z

Remaining compiler warnings — items needing design decisions

After the mechanical fixes pushed so far, here's what remains and why each needs discussion:

1. MSVC C4996: `std::is_pod` deprecated in C++20 (454 warnings in `CMemoryFwd.h`)

CMemoryFwd.h uses std::is_pod<T> extensively in memory tracking traits. std::is_pod was deprecated in C++20 and removed in C++26. The replacement is std::is_trivially_copyable<T> && std::is_trivially_default_constructible<T>, but is_pod has subtly different semantics in some edge cases.

Options:

Replace with the conjunction — most portable, but needs verification that behavior is identical for all our types
Suppress C4996 specifically for this header — quick fix but defers the real work
Create a project-level ml::core::is_pod_v alias that wraps the replacement

2. MSVC C4250: diamond inheritance (92 warnings in `CDataFrameAnalysisInstrumentation.h`)

The instrumentation class hierarchy has diamond inheritance where derived classes inherit the same method from two paths. MSVC warns about which path "wins" via dominance. The code works correctly but the design should be reviewed.

Options:

Suppress C4250 project-wide (it's purely informational and the dominance resolution is correct)
Refactor the hierarchy to avoid the diamond — larger change

3. GCC -Wsubobject-linkage (23 warnings in 4 test files)

Test fixtures defined in anonymous namespaces are used as Boost.Test base classes. GCC warns because the fixture type has internal linkage but the test class (generated by Boost macros) has external linkage.

Options:

Move fixtures out of anonymous namespaces into a named detail namespace
Suppress -Wno-subobject-linkage in test builds only

4. GCC/MSVC deprecated-enum-enum-conversion / C5054 (14 warnings in 3 test files)

Bitwise OR between CResultType::EConditionalOrUnconditional and another enum. This is deprecated in C++20 (different enum types in bitwise ops).

Options:

Add an explicit cast: static_cast<int>(a) | static_cast<int>(b)
Add a helper function in the CResultType class that does the combination
This is test code only — could suppress for test builds

The test deliberately divides by zero to verify edge-case handling in CMathsFuncs. Suppress C4723 for this file only. Made-with: Cursor

BOOST_TEST_MODULE and BOOST_TEST_NO_MAIN are consumed by the subsequent #include <boost/test/unit_test.hpp> but Clang flags them as unused since they're not referenced directly in source. This is a known false positive with Boost.Test's macro-driven configuration pattern. Made-with: Cursor

BOOST_TEST_MODULE and BOOST_TEST_NO_MAIN are consumed by the subsequent #include <boost/test/unit_test.hpp> but Clang flags them as unused since they're not referenced directly in source. Suppress for test targets only via target_compile_options in ml_add_test_executable, rather than globally. Made-with: Cursor

edsavage added :ml >refactoring labels Mar 12, 2026

edsavage added 4 commits March 13, 2026 14:04

[ML] Run clang-format on compiler warning fixes

1edba0a

Made-with: Cursor

Merge remote-tracking branch 'upstream/main' into fix/compiler-warnings

0f382ef

Made-with: Cursor # Conflicts: # bin/pytorch_inference/Main.cc

[ML] Fix deprecated implicit this capture via [=] in C++20

27e3df7

Change [=] to [this, f] in CConcurrentWrapper to silence -Wdeprecated on GCC 13 and Clang. Made-with: Cursor

Formatting

5e44996

edsavage requested a review from Copilot March 22, 2026 21:32

Copilot started reviewing on behalf of edsavage March 22, 2026 21:33 View session

Copilot AI reviewed Mar 22, 2026

View reviewed changes

edsavage requested a review from Copilot April 7, 2026 22:15

Copilot started reviewing on behalf of edsavage April 7, 2026 22:15 View session

Copilot AI reviewed Apr 7, 2026

View reviewed changes

[ML] Fix category count comparison to iterate map keys

76a6eb1

Address Copilot review: the loop assumed contiguous integer category IDs (0..N-1) and used operator[] which silently inserts missing keys. Iterate over actual map entries instead. Made-with: Cursor

edsavage force-pushed the fix/compiler-warnings branch from 31f4f21 to 76a6eb1 Compare April 8, 2026 01:10

edsavage added 3 commits April 8, 2026 13:38

[ML] Fix remaining [=] implicit this captures for C++20

505dcf2

Change [=] to [=, this] in 6 lambdas that capture this. Implicit this capture via [=] is deprecated in C++20. (GCC -Wdeprecated) Made-with: Cursor

[ML] Fix implicit size_t to double conversions

43045d9

Add static_cast<double> at 7 locations where size_t values are implicitly converted to double. (GCC -Wconversion, MSVC C4244/C4267) Made-with: Cursor

edsavage added 3 commits April 8, 2026 13:59

[ML] Suppress MSVC C4723 in CMathsFuncsTest

63e3e85

The test deliberately divides by zero to verify edge-case handling in CMathsFuncs. Suppress C4723 for this file only. Made-with: Cursor

edsavage force-pushed the fix/compiler-warnings branch from 7295264 to 4b7e9a5 Compare April 8, 2026 02:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Fix compiler warnings across the codebase#2985

[ML] Fix compiler warnings across the codebase#2985
edsavage wants to merge 13 commits intoelastic:mainfrom
edsavage:fix/compiler-warnings

edsavage commented Mar 12, 2026 •

edited

Loading

Uh oh!

prodsecmachine commented Mar 12, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 7, 2026

Uh oh!

edsavage commented Apr 8, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

edsavage commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

prodsecmachine commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Snyk checks have passed. No issues have been found so far.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

Uh oh!

edsavage commented Apr 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Remaining compiler warnings — items needing design decisions

1. MSVC C4996: std::is_pod deprecated in C++20 (454 warnings in CMemoryFwd.h)

2. MSVC C4250: diamond inheritance (92 warnings in CDataFrameAnalysisInstrumentation.h)

3. GCC -Wsubobject-linkage (23 warnings in 4 test files)

4. GCC/MSVC deprecated-enum-enum-conversion / C5054 (14 warnings in 3 test files)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

edsavage commented Mar 12, 2026 •

edited

Loading

prodsecmachine commented Mar 12, 2026 •

edited

Loading

edsavage commented Apr 8, 2026 •

edited

Loading

1. MSVC C4996: `std::is_pod` deprecated in C++20 (454 warnings in `CMemoryFwd.h`)

2. MSVC C4250: diamond inheritance (92 warnings in `CDataFrameAnalysisInstrumentation.h`)