Skip to content

refactor(clang-tidy): behaviour-preserving modernize sweep of CoolPropLib.cpp (CoolProp-jtl)#2985

Merged
ibell merged 1 commit into
masterfrom
ihb/clang-tidy-coolproplib-jtl
May 25, 2026
Merged

refactor(clang-tidy): behaviour-preserving modernize sweep of CoolPropLib.cpp (CoolProp-jtl)#2985
ibell merged 1 commit into
masterfrom
ihb/clang-tidy-coolproplib-jtl

Conversation

@ibell
Copy link
Copy Markdown
Contributor

@ibell ibell commented May 25, 2026

Resolves beads CoolProp-jtl. Continues the #2926 / CoolProp-2uw clang-tidy modernize series.

What

Clears the 24 whole-file clang-tidy signal findings in src/CoolPropLib.cpp that were surfaced while running preflight for #2873 (PR #2983). All changes are behaviour-preserving.

Check Sites Fix
clang-analyzer-security.insecureAPI.strcpy 7 strcpystd::memcpy(dst, s.c_str(), s.size()+1)
readability-implicit-bool-conversion 6 return true/false1/0; explicit static_cast for bool→long/int
modernize-raw-string-literal 4 escaped-quote format strings → R"(...)"
modernize-use-scoped-lock 3 std::lock_guard<std::mutex>std::scoped_lock
*-use-default-member-init 1 AbstractStateLibrary::next_handle in-class init + ctor = default
performance-unnecessary-value-param 1 add() takes shared_ptr by const ref
modernize-use-auto 1 map iterator → auto
cppcoreguidelines-special-member-functions 1 fpu_reset_guard declares deleted copy/move (RAII scope guard)

Why the strcpy → memcpy change is safe (not just silencing)

Every strcpy site was already guarded by a preceding if (s.size() < buffer_length) with strict < against the buffer-length variable that bounds the destination buffer. So s.size() + 1 <= buffer_length, and memcpy of size + null terminator always fits — byte-identical behaviour, minus the categorically-banned strcpy. (Confirmed per-site, including AbstractState_fluid_param_string correctly using return_buffer_length rather than the unrelated message_buffer/buffer_length.)

Deliberately out of scope

cppcoreguidelines-pro-bounds-pointer-arithmetic findings are left untouched — they sit in preflight's noise-filter and CI does not enforce them on this inherently pointer-based array C API.

Verification

  • ✅ Preflight green: clang-format, build, Catch2 tests, cppcheck, clang-tidy 0 signal findings, semgrep.
  • ✅ Adversarial pre-PR review: no blocking findings (memcpy guard-correspondence, raw-string byte-equality, scoped_lock semantics, const-ref safety, and fpu_reset_guard never-copied all individually verified).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved error message handling and reporting reliability
    • Enhanced buffer safety across library operations
    • Increased stability through safer memory handling in state-setter functions

Review Change Stack

#2926)

Behaviour-preserving clang-tidy cleanup of the C API translation unit,
clearing the 24 whole-file signal findings surfaced while running
preflight for #2873:

- insecureAPI.strcpy (7): strcpy -> std::memcpy(dst, s.c_str(), s.size()+1).
  Every site was already guarded by `if (s.size() < buffer_length)`, with
  strict `<` against the buffer length that bounds the destination, so the
  copy of size+null always fits. Behaviour identical; just drops the
  categorically-banned strcpy.
- readability-implicit-bool-conversion (6): explicit returns/casts
  (return true/false -> 1/0 in int APIs; static_cast for bool->long/int).
- modernize-raw-string-literal (4): escaped-quote format strings -> R"(...)".
- modernize-use-scoped-lock (3): std::lock_guard -> std::scoped_lock.
- use-default-member-init (1): AbstractStateLibrary::next_handle in-class
  init + ctor = default.
- performance-unnecessary-value-param (1): add() takes shared_ptr by const ref.
- modernize-use-auto (1): map iterator -> auto.
- cppcoreguidelines-special-member-functions (1): fpu_reset_guard declares
  deleted copy/move (it is an RAII scope guard, never copied).

pro-bounds-pointer-arithmetic findings are intentionally left untouched —
they are in preflight's noise-filter (CI does not enforce them on this
inherently pointer-based array C API).

Preflight green (build, tests, cppcheck, clang-tidy 0 signal, semgrep);
adversarial pre-PR review found no blocking issues.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 25, 2026

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f296c9d-6be0-4f44-9ce3-7f2603dfe0ea

📥 Commits

Reviewing files that changed from the base of the PR and between e92a71a and 1d255c7.

📒 Files selected for processing (1)
  • src/CoolPropLib.cpp

📝 Walkthrough

Walkthrough

This PR improves buffer safety and type consistency in CoolProp's C-API layer (src/CoolPropLib.cpp). Error handling now uses bounded memcpy instead of unsafe strcpy, internal handle management is refactored with safer synchronization, reference-state setters return integer codes for error indication, and multiple string-output functions apply the same safe-copy pattern. Error messages adopt raw-string literals for clarity.

Changes

CoolProp C-API Buffer Safety and Type Safety

Layer / File(s) Summary
Error handling and resource lifetime
src/CoolPropLib.cpp
HandleException replaces strcpy with std::memcpy for writing exception messages into message_buffer under existing length checks. fpu_reset_guard is made explicitly non-copyable and non-movable by deleting copy/move constructors and assignment operators.
AbstractStateLibrary handle management refactoring
src/CoolPropLib.cpp
AbstractStateLibrary is refactored to default-initialize next_handle as a member, use std::scoped_lock for synchronization, and pass shared_ptr by const reference in the add method instead of by value.
String buffer copying safety across C-API functions
src/CoolPropLib.cpp
AbstractState_fluid_names, AbstractState_backend_name, AbstractState_fluid_param_string, and C_extract_backend replace strcpy with std::memcpy when writing composed or formatted strings into caller-provided buffers, copying size()+1 bytes under existing bounds checks.
Reference state setter return codes
src/CoolPropLib.cpp
set_reference_stateS and set_reference_stateD change from void functions to return integer success codes (1 on success, 0 on failure), while preserving exception handling and error-string assignment in catch blocks.
String formatting and output type casts
src/CoolPropLib.cpp
get_parameter_information_string adopts raw-string literal formatting for error messages; AbstractState_get_mole_fractions_satState and AbstractState_keyed_output_satState update error messages to raw-string literals; AbstractState_all_critical_points casts pts[i].stable to long when writing output; C_is_valid_fluid_string explicitly casts boolean result to int.

Poem

🐰 Whiskers twitching with delight—
Buffer bounds now safe and tight!
No more strcpy troubles brew,
Memcpy takes the safer route true.
Types cast clear, handles shine,
CoolProp's C-API now divine!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: a modernization refactoring of CoolPropLib.cpp to fix clang-tidy findings while preserving behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ihb/clang-tidy-coolproplib-jtl

Comment @coderabbitai help to get the list of available commands and usage tips.

@ibell ibell merged commit bdbe959 into master May 25, 2026
20 of 22 checks passed
@ibell ibell deleted the ihb/clang-tidy-coolproplib-jtl branch May 25, 2026 11:21
@ibell ibell added this to the v8.0.0 milestone May 27, 2026
ibell added a commit that referenced this pull request May 27, 2026
…yer 2) (#3018)

Hand-written high-signal fixes from the layer-2 clang-tidy sweep
(CoolProp-1pr), the follow-up to the #2926 triage after the first fix
wave landed (#2978/#2980/#2983/#2985).

- bugprone-exception-escape (FlashRoutines.cpp): wrap the ~solver_resid()
  RAII cleanup in try/catch.  The dtor calls HEOS->unspecify_phase();
  a throw escaping a destructor (implicitly noexcept) calls
  std::terminate.  unspecify_phase() only resets a flag and shouldn't
  throw, but the guard makes the no-crash guarantee explicit.

- bugprone-switch-missing-default-case x5 (HelmholtzEOSMixtureBackend.cpp):
  add default: break; to the four calc_saturation_ancillary Q-switches
  (they fall through to the existing "Q invalid" throw) and to the
  iDmolar phase-guess switch (falls through to the full VLE path).
  No behavior change; documents the unhandled arms.

- bugprone-assignment-in-if-condition x3 (HumidAirProp.cpp): replace the
  if((key=get_input_key(...))>=0){} else-if chain (empty bodies, side-
  effecting condition) with sequential first-match assignment.  Same
  priority order (HUMRAT > TDP > RH) and same throw on no match.

- bugprone-macro-parentheses x2 (Configuration.cpp): parenthesize the
  String macro argument in the CONFIGURATION_KEYS_ENUM X-macro.

Tests: [HAProps]/[humid_air*]/[ancillary]/[saturation] (4608 assertions,
23 cases) and the preflight [!slow][!benchmark] suite pass.

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant