Skip to content

clang-tidy: resolve performance-inefficient-string-concatenation#585

Open
greenc-FNAL wants to merge 9 commits into
mainfrom
maintenance/clang-tidy/performance-inefficient-string-concatenation
Open

clang-tidy: resolve performance-inefficient-string-concatenation#585
greenc-FNAL wants to merge 9 commits into
mainfrom
maintenance/clang-tidy/performance-inefficient-string-concatenation

Conversation

@greenc-FNAL
Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings May 13, 2026 19:31
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR resolves the performance-inefficient-string-concatenation clang-tidy warnings in two locations by restructuring string-building expressions to avoid creating temporary string objects from chained operator+ calls.

Changes:

  • In pymodule.cpp, inline the Python version components directly into the single concatenation expression for site_packages.
  • In framework_graph.cpp, use chained std::string::append calls instead of operator+ when building the runtime_error message.
  • Mark the corresponding clang-tidy check as completed in the tracking document, linking to this PR.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
plugins/python/src/pymodule.cpp Collapses version-string construction into the site_packages concatenation.
phlex/core/framework_graph.cpp Rewrites error message construction using std::string::append chain.
docs/dev/clang-tidy-fixes-2026-04.md Marks the check as resolved and links to PR #585.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #585      +/-   ##
==========================================
+ Coverage   82.62%   82.67%   +0.04%     
==========================================
  Files         161      161              
  Lines        5895     5894       -1     
  Branches      682      682              
==========================================
+ Hits         4871     4873       +2     
+ Misses        803      801       -2     
+ Partials      221      220       -1     
Flag Coverage Δ
scripts 76.12% <ø> (ø)
unittests 86.05% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
phlex/core/framework_graph.cpp 94.64% <100.00%> (+2.67%) ⬆️
plugins/python/src/pymodule.cpp 58.16% <100.00%> (-0.43%) ⬇️

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3455282...6d1735f. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread phlex/core/framework_graph.cpp Outdated
@knoepfel
Copy link
Copy Markdown
Member

@phlexbot format

@github-actions
Copy link
Copy Markdown
Contributor

Format Fixes Applied

✅ clang-format fixes pushed (commit 3a73e3d)

⚠️ Note: Some issues may require manual review.

@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

@knoepfel Is there a reason C++23 std::format wouldn't work here?

@knoepfel
Copy link
Copy Markdown
Member

@knoepfel Is there a reason C++23 std::format wouldn't work here?

I'd have to check whether std::format is supported on apple-clang. We're already using fmt::format in various places, so I figured that was reasonable. I actually didn't intend for copilot to make any changes—I was just asking if fmt::format would be better. Lo and behold, it made the change (with the wrong code formatting).

@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

greenc-FNAL commented May 13, 2026

@knoepfel Is there a reason C++23 std::format wouldn't work here?

I'd have to check whether std::format is supported on apple-clang. We're already using fmt::format in various places, so I figured that was reasonable. I actually didn't intend for copilot to make any changes—I was just asking if fmt::format would be better. Lo and behold, it made the change (with the wrong code formatting).

Yeah, I think you need to change it to "ask" instead of "agent," maybe?

test/form/reader.cpp and test/form/writer.cpp already use std::format.

I've been using it if it works, bearing in mind that some things may not be there yet.

@knoepfel
Copy link
Copy Markdown
Member

test/form/reader.cpp and test/form/writer.cpp already use std::format.

I've been using it if it works, bearing in mind that some things may not be there yet.

Ah, I didn't realize this. In that case, std::format would be fine. Do you want to make the change?

@greenc-FNAL greenc-FNAL force-pushed the maintenance/clang-tidy/performance-inefficient-string-concatenation branch from 3a73e3d to 5ab22b6 Compare May 13, 2026 21:05
@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

@copilot Please add/adjust tests to ensure that untested code paths touched by this PR are tested to ensure correct operation under expected circumstances.

Copy link
Copy Markdown
Contributor

Copilot AI commented May 13, 2026

@copilot Please add/adjust tests to ensure that untested code paths touched by this PR are tested to ensure correct operation under expected circumstances.

Added in commit f39c117: I introduced a focused test in test/framework_graph.cpp that exercises the previously untested path where a consumer specifies a non-existent predicate via experimental_when(...), and verifies the expected runtime error message. No UI changes in this PR.

@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

@phlexbot format

@github-actions
Copy link
Copy Markdown
Contributor

Format Fixes Applied

✅ clang-format fixes pushed (commit 04cf7f3)

⚠️ Note: Some issues may require manual review.

@greenc-FNAL greenc-FNAL force-pushed the maintenance/clang-tidy/performance-inefficient-string-concatenation branch from 04cf7f3 to dcf84dd Compare May 19, 2026 14:22
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this merge got mangled (removing the record of fixes already performed).

@greenc-FNAL greenc-FNAL force-pushed the maintenance/clang-tidy/performance-inefficient-string-concatenation branch from dcf84dd to deba02b Compare May 20, 2026 20:12
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.

4 participants