Skip to content

clang-tidy: resolve bugprone-implicit-widening-of-multiplication-result#496

Open
greenc-FNAL wants to merge 6 commits intomainfrom
maintenance/clang-tidy/bugprone-implicit-widening-of-multiplication-result
Open

clang-tidy: resolve bugprone-implicit-widening-of-multiplication-result#496
greenc-FNAL wants to merge 6 commits intomainfrom
maintenance/clang-tidy/bugprone-implicit-widening-of-multiplication-result

Conversation

@greenc-FNAL
Copy link
Copy Markdown
Contributor

@greenc-FNAL greenc-FNAL commented Apr 8, 2026

Multiplications in test assertions and a compile-time array extent were done in unsigned int before widening to std::size_t. Cast one operand before multiplying so arithmetic happens in the correct type throughout.

Before:

CHECK(g.execution_count("square") == std::size_t(index_limit * number_limit));
std::array<double, 3ull * 1024ull> samples;

After:

CHECK(g.execution_count("square") == std::size_t{index_limit} * number_limit);
std::array<double, std::size_t{3} * 1024> samples;
  • test/hierarchical_nodes.cpp, test/fold.cpp, test/different_hierarchies.cpp, test/cached_execution.cpp: Replace std::size_t(a * b) with std::size_t{a} * b in execution_count assertions.
  • test/demo-giantdata/waveforms.hpp: Replace 3ull * 1024ull array extent with std::size_t{3} * 1024.
  • docs/dev/clang-tidy-fixes-2026-04.md: Mark check as resolved and link this PR, consistent with other completed entries.

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 addresses clang-tidy’s bugprone-implicit-widening-of-multiplication-result findings by making std::size_t-typed expectations explicit in several tests (plus one compile-time array extent), and updates the April 2026 clang-tidy fixes tracking doc accordingly.

Changes:

  • Updated several CHECK(g.execution_count(...) == ...) expectations to convert products to std::size_t.
  • Adjusted a std::array extent expression in the giantdata demo to use unsigned-long-long literals.
  • Marked bugprone-implicit-widening-of-multiplication-result as complete in docs/dev/clang-tidy-fixes-2026-04.md.

Reviewed changes

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

Show a summary per file
File Description
test/hierarchical_nodes.cpp Casts multiplication result used in execution-count expectations to std::size_t.
test/fold.cpp Casts multiplication result used in execution-count expectations to std::size_t.
test/different_hierarchies.cpp Casts one execution-count product expectation to std::size_t.
test/demo-giantdata/waveforms.hpp Changes std::array extent to use ull literals in the multiplication.
test/cached_execution.cpp Casts multi-factor execution-count expectations to std::size_t.
docs/dev/clang-tidy-fixes-2026-04.md Checks off the clang-tidy item for this warning class.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 8, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #496      +/-   ##
==========================================
- Coverage   85.57%   85.54%   -0.03%     
==========================================
  Files         142      142              
  Lines        3604     3604              
  Branches      616      616              
==========================================
- Hits         3084     3083       -1     
  Misses        310      310              
- Partials      210      211       +1     
Flag Coverage Δ
unittests 85.54% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 1 file 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 51fc599...1e13c21. Read the comment docs.

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

Copy link
Copy Markdown
Member

@knoepfel knoepfel left a comment

Choose a reason for hiding this comment

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

Thanks, @greenc-FNAL. A couple questions below.


CHECK(g.execution_count("B1") == n_runs * n_subruns);
CHECK(g.execution_count("B2") == n_runs * n_subruns);
CHECK(g.execution_count("B1") == std::size_t{n_runs} * n_subruns);
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.

I wonder if a large swath of these warnings can be fixed by just changing the type of (e.g.) n_runs to std::size_t.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would probably work, but in general I was trying to avoid semantic changes to the tests wherever possible. Let me know if you'd like me to go ahead and make this change.

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.

Understood. I think this is an example where clang-tidy identified a potential problem, and the cleanest solution is to adjust the type of n_runs, etc.

greenc-FNAL and others added 4 commits April 9, 2026 14:33
…esult`

Some tests multiply integer constants or variables in contexts where the
result is expected to be `std::size_t`: we resolve the check complaints
by explicitly converting to `std::size_t`.
…sult after; add PR link to docs

Agent-Logs-Url: https://github.com/Framework-R-D/phlex/sessions/75b911b7-81ef-400f-bbd6-f641fb353276

Co-authored-by: greenc-FNAL <2372949+greenc-FNAL@users.noreply.github.com>
@greenc-FNAL greenc-FNAL force-pushed the maintenance/clang-tidy/bugprone-implicit-widening-of-multiplication-result branch from 59eeffc to 1e13c21 Compare April 9, 2026 19:33
knoepfel
knoepfel previously approved these changes Apr 10, 2026
@knoepfel knoepfel self-requested a review April 10, 2026 18:28
@knoepfel knoepfel dismissed their stale review April 10, 2026 18:28

The tests have failed, likely due to a mismatch in types.

@greenc-FNAL
Copy link
Copy Markdown
Contributor Author

@knoepfel Should I revert?

@knoepfel
Copy link
Copy Markdown
Member

@knoepfel Should I revert?

Yes, let's revert those changes for now.

Type change propagates to persistent types, causing test failures: too many changes required.

This reverts commit d194b63.
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