refactor: apply safe clang-tidy --fix passes (CoolProp-2uw.13, part 1) by ibell · Pull Request #2805 · CoolProp/CoolProp

ibell · 2026-04-26T15:14:37Z

Summary

Tier 4.2 of the C++ code-quality epic (CoolProp-2uw), first installment. Mechanically applies the auto-fixable subset of three modernize-* checks via run-clang-tidy --fix in an Ubuntu 24.04 + clang-tidy-18 container.

Checks applied

Check	Raw diagnostics	What it does
`modernize-use-nullptr`	21	`NULL` / `0` → `nullptr` for pointer types
`modernize-use-emplace`	53	`push_back(T(args))` → `emplace_back(args)`
`modernize-deprecated-headers`	24	`<math.h>` → `<cmath>`, `<stdlib.h>` → `<cstdlib>`, `<stdio.h>` → `<cstdio>`, `<string.h>` → `<cstring>`, `<time.h>` → `<ctime>`, `<float.h>` → `<cfloat>`; quoted variants handled too

Note: the diagnostic counts are clang-tidy's raw output (some duplicated when a header is included from multiple TUs). The actual unique fixes are the 24-file +43/−43 diff below.

Pure mechanical modernization. cmake --build build --target format ran after the tidy fixes; no formatting drift surfaced.

What was deliberately skipped this round

cppcoreguidelines-init-variables (1058 raw diagnostics — would add = NAN to every uninitialized double). Two reasons:

Header conflict: the check's MathHeader option defaults to <math.h>, which modernize-deprecated-headers then immediately wants to remove. Running both together produced bogus diffs with <math.h> and <cmath> both included. Need to set cppcoreguidelines-init-variables.MathHeader = <cmath> in .clang-tidy first.
Diff size + behavior change: 1000+ NAN initializations is large enough to deserve its own focused review. NAN init is actually defensible (propagates as a tracer when reads happen before assignment) but worth reviewing per-call-site for solver paths.

Plan: small follow-up PR adds the MathHeader = <cmath> CheckOption, then a separate PR applies the init-variables fix cleanly.

Verification

All include changes are 1:1 swaps (43 insertions, 43 deletions; no orphaned includes)
Format target run post-fix produced no additional drift
ASan / build CI on this PR will validate compilation

Follow-up

Append the squash SHA of this PR to .git-blame-ignore-revs (separate small PR after merge)
cppcoreguidelines-init-variables.MathHeader = <cmath> config tweak + the init-variables fix pass

🤖 Generated with Claude Code

Tier 4.2 of the C++ code-quality epic, first installment. Mechanically applies the auto-fixable subset of three modernize-* checks via run-clang-tidy --fix in an Ubuntu 24.04 + clang-tidy-18 container. Checks applied (counts are clang-tidy raw diagnostics; some are duplicates from headers seen across multiple TUs — actual unique fixes are the 24-file +43/-43 diff): modernize-use-nullptr : 21 (NULL/0 → nullptr for pointer types) modernize-use-emplace : 53 (push_back(T(args)) → emplace_back(args)) modernize-deprecated-headers : 24 (<math.h> → <cmath>, <stdlib.h> → <cstdlib>, <stdio.h> → <cstdio>, <string.h> → <cstring>, <time.h> → <ctime>, <float.h> → <cfloat>; quoted variants also handled) Net diff: 24 files, +43/-43 (1:1 line replacements throughout). Pure mechanical modernization. cmake --build build --target format ran after fixes; no formatting drift surfaced. Skipped this round (will be follow-up PRs after .clang-tidy tuning): cppcoreguidelines-init-variables (1058 raw diagnostics): adds = NAN to uninitialized doubles. Conflicts with modernize-deprecated-headers in this run because the fix's MathHeader option defaults to <math.h>, which the deprecated-headers check then immediately wants to remove. Need to set cppcoreguidelines-init-variables.MathHeader=<cmath> in .clang-tidy first, then apply this check separately. Also the 1000+ NAN-init diff is large enough to deserve its own focused review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…p-2uw.13b) (#2807) - Append squash SHA of PR #2805 (safe clang-tidy --fix passes: modernize-use-nullptr, modernize-use-emplace, modernize-deprecated-headers) to .git-blame-ignore-revs so git blame walks through that mechanical churn. - Add cppcoreguidelines-init-variables.MathHeader=<cmath> CheckOption to .clang-tidy so the upcoming part-2 NAN-init pass uses <cmath> for include placement, matching the deprecated-headers cleanup already in tree. Closes CoolProp-86n Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

) Follow-up to CoolProp-2uw.13: the original Tier 4.2 plan included modernize-use-override but only nullptr/emplace/deprecated-headers (#2805) and init-variables (#2808) actually landed. Local builds were still emitting -Winconsistent-missing-override on every class with a partial override pattern; this sweep closes that. Approach: re-enable modernize-use-override in .clang-tidy (the existing "project uses override+final via macros" comment was stale — no such macros exist, the codebase uses bare `override`), then run run-clang-tidy -p build_catch \ -checks='-*,modernize-use-override' \ -config="{CheckOptions: [ {key: modernize-use-override.AllowOverrideAndFinal, value: 'true'}, {key: modernize-use-override.IgnoreDestructors, value: 'true'}, ]}" \ -header-filter='^.../(include|src)/' \ -fix -format -j 1 across the whole tree. clang-tidy re-emits identical fix-its from every TU that includes a given header, so collapse adjacent `override override` keywords post-pass (mechanical, idempotent). Effects: - 34 first-party files touched (~650 lines either way; net zero — only keyword additions on existing declarations / redundant `virtual` removed on overrides). - 0 -Winconsistent-missing-override warnings on a clean build (was pervasive across Cubics/Helmholtz/IF97/PCSAFT/REFPROP/Tabular). - .clang-tidy: enable check + add tuning CheckOptions (AllowOverrideAndFinal, IgnoreDestructors) matching the historical 2uw.13 plan. Verified: ./dev/ci/preflight.sh passes (Catch [Helmholtz],[REFPROP], [!benchmark] tag scope auto-selected from the changed paths; cppcheck + clang-tidy skipped because preflight runs them whole-file and they flag long-standing issues unrelated to this diff — CI runs clang-tidy-diff which only sees changed lines). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…2926 layer 2) Mechanical clang-tidy fix-it sweep from the layer-2 triage (CoolProp-1pr), the leftover modernize/perf batch that #2980 did not run. Applied with per-file sequential `clang-tidy -fix` (one TU at a time, so a fix to a shared header by the first includer is already present when later TUs parse it -> no cross-TU double application) and an anchored header-filter (^<repo>/(include|src)/) so build_catch/_deps third-party trees are never touched. Formatted with the pinned clang-format 18.1.8. Checks applied (override the .clang-tidy whitelist for this run): performance-unnecessary-value-param (pass containers/strings by const&) cppcoreguidelines-prefer-member-initializer (ctor-body assign -> init list) modernize-use-auto (auto for new / iterators) modernize-loop-convert (range-based for) modernize-return-braced-init-list modernize-raw-string-literal modernize-use-emplace modernize-use-equals-default ({} -> = default) modernize-use-using (typedef -> using) performance-unnecessary-copy-initialization (copy -> const&) 70 files, mechanical only; no signature changes to virtual functions. Caught-and-reverted regression (adversarial review): the prefer-member-initializer fix wrongly hoisted `N = n.size(); s = n;` into the SurfaceTensionCorrelation(rapidjson::Value&) member-init list (include/Ancillaries.h). `n` is populated in the constructor body, so at init-list time it is empty -> N=0, s={} -> evaluate() silently returns 0.0 for every fluid's surface tension. Reverted that one ctor to assign N/s in the body (NOLINT + comment), and added a [surface_tension] regression test (none existed, which is why the full suite missed it) checking water sigma against IAPWS at 300/350 K. Vendored include/superancillary/superancillary.h carries trivial =default/auto fixes (Ian OK'd editing it); the same should also go upstream to avoid a re-vendor clobber. Tests: full ~[slow]~[benchmark] suite green (297 cases, 117580 assertions) plus the new [surface_tension] case. Recommend adding this commit to .git-blame-ignore-revs (mechanical sweep), matching #2803/#2805/#2808/#2980. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…3019) * refactor(clang-tidy): Tier-2 mechanical fix-it sweep (CoolProp-0xu, #2926 layer 2) Mechanical clang-tidy fix-it sweep from the layer-2 triage (CoolProp-1pr), the leftover modernize/perf batch that #2980 did not run. Applied with per-file sequential `clang-tidy -fix` (one TU at a time, so a fix to a shared header by the first includer is already present when later TUs parse it -> no cross-TU double application) and an anchored header-filter (^<repo>/(include|src)/) so build_catch/_deps third-party trees are never touched. Formatted with the pinned clang-format 18.1.8. Checks applied (override the .clang-tidy whitelist for this run): performance-unnecessary-value-param (pass containers/strings by const&) cppcoreguidelines-prefer-member-initializer (ctor-body assign -> init list) modernize-use-auto (auto for new / iterators) modernize-loop-convert (range-based for) modernize-return-braced-init-list modernize-raw-string-literal modernize-use-emplace modernize-use-equals-default ({} -> = default) modernize-use-using (typedef -> using) performance-unnecessary-copy-initialization (copy -> const&) 70 files, mechanical only; no signature changes to virtual functions. Caught-and-reverted regression (adversarial review): the prefer-member-initializer fix wrongly hoisted `N = n.size(); s = n;` into the SurfaceTensionCorrelation(rapidjson::Value&) member-init list (include/Ancillaries.h). `n` is populated in the constructor body, so at init-list time it is empty -> N=0, s={} -> evaluate() silently returns 0.0 for every fluid's surface tension. Reverted that one ctor to assign N/s in the body (NOLINT + comment), and added a [surface_tension] regression test (none existed, which is why the full suite missed it) checking water sigma against IAPWS at 300/350 K. Vendored include/superancillary/superancillary.h carries trivial =default/auto fixes (Ian OK'd editing it); the same should also go upstream to avoid a re-vendor clobber. Tests: full ~[slow]~[benchmark] suite green (297 cases, 117580 assertions) plus the new [surface_tension] case. Recommend adding this commit to .git-blame-ignore-revs (mechanical sweep), matching #2803/#2805/#2808/#2980. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(clang-tidy): use auto in CoolPropDbl range loops (CodeRabbit, #3019) loop-convert in the Tier-2 sweep baked the concrete element type `double` into range-for loops over std::vector<CoolPropDbl>, deduced from this build's default config (COOLPROPDBL_MAPS_TO_DOUBLE). When CoolPropDbl is long double, a non-const `double&` can't bind to the elements (hard compile error) and by-value `double` silently narrows. CodeRabbit flagged one site (PCSAFTBackend.cpp); the same defect was in four more. Switch all CoolPropDbl-backed loops to `auto`, which is what loop-convert should have emitted: - FluidLibrary.h: `auto&` (mutates: flips sign of theta) [was double&] - PCSAFTBackend.cpp: `const auto&` (read-only) [was double&] - IncompressibleBackend.cpp x3: `auto` (mole/mass/volu) [was double] Genuine std::vector<double> loops (rapidjson helpers, CPnumerics, the PropsSImulti IO buffer, test vectors) are left as-is — those bind correctly regardless of CoolPropDbl. Full ~[slow]~[benchmark] suite still green (298 cases, 117586 assertions). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(clang-tidy): address review nits on Tier-2 sweep (#3019) - GeneralizedCubic.h/VTPRCubic.h: drop the misleading `std::move(Tc)` in PengRobinson/SRK/VTPRCubic ctors. Tc is `const std::vector<double>&`, so `std::move` silently degrades to a copy (can't move from const). AbstractCubic stores Tc by copy from a const& param anyway; the std::move was a no-op that read as if it actually moved. Kept `std::move(pc)` and `std::move(acentric)` — those params are by-value, so the move is real. - FluidLibrary.cpp: rename loop variable `aliase` -> `alias`. - TabularBackends.h: remove stale "// Flush the cached indices" comment; the cached_*_i/j members are now initialized in the member-init list (Tier-2 sweep moved them), so the comment in the empty ctor body was pointing at code that no longer exists in the body. - Helmholtz.cpp: modernize `std::string derivs[]` C-array -> `const std::vector<std::string>` (loop is already range-based; C++17, so not constexpr). - CoolProp-Tests.cpp: rename `mixe` -> `mix`, `entrie` -> `entry` (x2); modernize `std::string Ykeys[]` C-array -> `const std::vector<std::string>`. Behavior-preserving; full ~[slow] fast suite (290 cases, 117,586 assertions) passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(FluidLibrary): rename second aliase->alias loop missed in 777cec4 Reviewer caught that the previous nit-fix commit renamed only the first of two identically-typo'd `aliase` loops in JSONFluidLibrary::add_one; the alias-registration loop (around L366) was still typo'd. Apply the same rename for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…ore-revs [skip ci] (#3027) #3019 (Tier-2 mechanical fix-it sweep, merged as fe950bb) is exactly the kind of behavior-preserving, whole-tree clang-tidy diff that should not pollute git blame. Adding alongside the prior precedent already in this file (#2803/#2805/#2808). Also adds #2980 (Tier-1: perf sweep + virtual-in-ctor + float-loops, 2e61d46) — same category, called out as a precedent in #3019's PR description but never added to .git-blame-ignore-revs at merge time. Skips CI: tooling-only file change, no code touched. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

ibell merged commit ee87978 into master Apr 26, 2026
1 check passed

ibell mentioned this pull request Apr 26, 2026

chore: blame-revs append + clang-tidy MathHeader (CoolProp-2uw.13b) #2807

Merged

3 tasks

This was referenced May 5, 2026

refactor(modernize-redundant-void-arg): drop (void) parameter lists (#2871) #2886

Merged

refactor(cert-err58-cpp): NOLINT static-init that may throw (#2874) #2887

Merged

ibell deleted the ib/clang-tidy-safe-fixes branch May 13, 2026 00:39

ibell mentioned this pull request May 24, 2026

refactor(clang-tidy): modernize-use-override sweep (CoolProp-7ue) #2976

Merged

4 tasks

ibell added this to the v8.0.0 milestone May 27, 2026

ibell mentioned this pull request May 27, 2026

refactor(clang-tidy): Tier-2 mechanical fix-it sweep (#2926 layer 2) #3019

Merged

ibell mentioned this pull request May 29, 2026

chore(blame): add #2980 + #3019 to .git-blame-ignore-revs [skip ci] #3027

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: apply safe clang-tidy --fix passes (CoolProp-2uw.13, part 1)#2805

refactor: apply safe clang-tidy --fix passes (CoolProp-2uw.13, part 1)#2805
ibell merged 1 commit into
masterfrom
ib/clang-tidy-safe-fixes

ibell commented Apr 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

ibell commented Apr 26, 2026

Summary

Checks applied

What was deliberately skipped this round

Verification

Follow-up

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant