Fixed issues with use-after-move - #1014
Merged
Merged
Conversation
volkm
commented
Aug 14, 2026
|
|
||
| // Get the solver object and satisfy requirements | ||
| // The solver consumes the matrix, so the acyclic check must be performed before moving it. | ||
| bool const hasCycles = storm::utility::graph::hasCycle(sccMatrix); |
Contributor
Author
There was a problem hiding this comment.
This check is now always performed. Is this an acceptable performance impact? Or do you have a better suggestion how to handle this part?
volkm
commented
Aug 14, 2026
| } | ||
| if (!choiceRewardsJson.empty()) { | ||
| choiceRewardsJson["rew"] = std::move(choiceRewardsJson); | ||
| choiceJson["rew"] = std::move(choiceRewardsJson); |
Contributor
Author
There was a problem hiding this comment.
This was incorrect before, but I guess the function was seldom used.
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR addresses clang-tidy’s bugprone-use-after-move findings, enables additional clang-tidy bugprone checks, and improves the clang-tidy script so file selection works reliably with relative paths.
Changes:
- Fixes multiple use-after-move patterns by capturing goal/config values before
std::move, and by restructuring JSON construction to avoid self-move / moved-from reuse. - Enables additional
bugprone-*clang-tidy checks in.clang-tidy. - Updates
resources/scripts/clang-tidy.shto correctly handle relative paths (cwd vs repo-root) and normalize paths for compilation database matching.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/storm/modelchecker/prctl/dtmc/DtmcPrctlModelCheckerTest.cpp | Adjusts test setup to avoid moving an already-consumed SolveGoal. |
| src/storm/storage/valuations/ValuationsStorage.cpp | Fixes assertions/logging to reference the actual stored members (this->strings, this->valuations). |
| src/storm/storage/memorystructure/SparseModelMemoryProduct.cpp | Uses components.transitionMatrix for row-grouping check (fixing incorrect variable usage). |
| src/storm/storage/jani/visitor/JSONExporter.cpp | Reworks array-access JSON building to avoid use-after-move of the intermediate JSON object. |
| src/storm/models/sparse/StochasticTwoPlayerGame.cpp | Adds suppression and comment for clang-tidy use-after-move warning in ctor init list. |
| src/storm/models/sparse/Smg.cpp | Adds suppression blocks for use-after-move warning due to base ctor consuming only part of components. |
| src/storm/models/sparse/Model.cpp | Fixes a self-move bug when attaching rewards JSON to a choice JSON object. |
| src/storm/models/sparse/MarkovAutomaton.cpp | Adds suppression blocks for use-after-move warning in ctor. |
| src/storm/models/sparse/Ctmc.cpp | Adds suppression blocks for use-after-move warning in ctor. |
| src/storm/modelchecker/prctl/helper/SparseMdpPrctlHelper.cpp | Captures goal-derived settings before moving goal; refactors scheduler extension API to avoid goal access post-move. |
| src/storm/modelchecker/prctl/helper/SparseDtmcPrctlHelper.cpp | Captures uncertainty mode before moving goal. |
| src/storm/modelchecker/helper/indefinitehorizon/visitingtimes/SparseDeterministicVisitingTimesHelper.cpp | Avoids using a moved matrix by computing cycle info before moving it to the solver. |
| src/storm-pars/modelchecker/region/monotonicity/MonotonicityResult.cpp | Removes pointless std::move of enum values and avoids map access patterns that can be problematic after moves. |
| src/storm-dft/generator/DftNextStateGenerator.cpp | Ensures a fresh choice object is used after adding a choice (avoids reusing moved-from state). |
| resources/scripts/clang-tidy.sh | Fixes file selection behavior for relative paths and normalizes absolute paths to match compilation DB entries. |
| .clang-tidy | Enables additional bugprone-* checks. |
Suppressed comments (1)
src/storm/models/sparse/Ctmc.cpp:1
- Same pattern as other model ctors: accessing
componentsafter moving it into the base ctor and suppressing the diagnostic. This is fragile if the base ctor’s implementation changes. Consider refactoring to avoid moved-from access (split derived-only fields from base-relevant fields) so this does not rely on undocumented consumption semantics, and/or narrow the suppression scope.
#include "storm/models/sparse/Ctmc.h"
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+40
to
+42
| : NondeterministicModel<ValueType, RewardModelType>(ModelType::S2pg, std::move(components)), | ||
| // NOLINTNEXTLINE(bugprone-use-after-move) The base constructor only consumes the base-relevant fields of components. | ||
| player1Matrix(std::move(components.player1Matrix.get())) { |
Comment on lines
+26
to
+32
| // NOLINTBEGIN(bugprone-use-after-move) The base constructor only consumes the base-relevant fields of components. | ||
| statePlayerIndications(std::move(components.statePlayerIndications.get())) { | ||
| if (components.playerNameToIndexMap) { | ||
| playerNameToIndexMap = std::move(components.playerNameToIndexMap.get()); | ||
| } | ||
| // Otherwise the map remains empty. | ||
| // NOLINTEND(bugprone-use-after-move) |
Comment on lines
52
to
+63
| @@ -59,6 +60,7 @@ MarkovAutomaton<ValueType, RewardModelType>::MarkovAutomaton(storm::storage::spa | |||
| if (components.rateTransitions) { | |||
| this->turnRatesToProbabilities(); | |||
| } | |||
| // NOLINTEND(bugprone-use-after-move) | |||
Comment on lines
+474
to
476
| // The solver consumes the matrix, so the acyclic check must be performed before moving it. | ||
| bool const hasCycles = storm::utility::graph::hasCycle(sccMatrix); | ||
| auto solver = linearEquationSolverFactory.create(env, std::move(sccMatrix)); |
| if (req.acyclic().isCritical()) { | ||
| STORM_LOG_THROW(!storm::utility::graph::hasCycle(sccMatrix), storm::exceptions::UnmetRequirementException, | ||
| "The solver requires an acyclic model, but the model is not acyclic."); | ||
| STORM_LOG_THROW(!hasCycles, storm::exceptions::UnmetRequirementException, "The solver requires an acyclic model, but the model is not acyclic."); |
sjunges
reviewed
Aug 14, 2026
Contributor
|
LGTM. |
Contributor
Author
|
The review comments by copilot are things I deliberately did not address. Splitting the components in the constructors would make the code unnecessarily complicated in my view. But if you have a good idea, I am happy to make changes. @tquatmann Is the performance impact in |
Contributor
|
Feel free to ignore copilot. Half of the suggestions are good, on average
....
…On Sat, 15 Aug 2026, 13:15 Matthias Volk, ***@***.***> wrote:
*volkm* left a comment (stormchecker/storm#1014)
<#1014 (comment)>
The review comments by copilot are things I deliberately did not address.
Splitting the components in the constructors would make the code
unnecessarily complicated in my view. But if you have a good idea, I am
happy to make changes.
@tquatmann <https://github.com/tquatmann> Is the performance impact in
SparseDeterministicVisitingTimesHelper okay?
—
Reply to this email directly, view it on GitHub
<#1014?email_source=notifications&email_token=ADH67DE7TINQCBVXO6NCL5L5KBA4PA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZQGE4TOMRQGI3KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5301972026>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADH67DCY4I6XHSQJT62HKYT5KBA4PAVCNFSNUABEKJSXA33TNF2G64TZHM3TCMZZGU2TANB3JFZXG5LFHM2TCNJRGIYDSNJZGCQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ADH67DB3VSEUWPB2UWFYVCL5KBA4PA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZQGE4TOMRQGI3KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ADH67DGMAW2D4ILRGONAIFD5KBA4PA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMZQGE4TOMRQGI3KM4TFMFZW63VHMNXW23LFNZ2KKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you commented.Message ID:
***@***.***>
|
tquatmann
reviewed
Aug 17, 2026
tquatmann
approved these changes
Aug 17, 2026
tquatmann
left a comment
tquatmann
left a comment
Contributor
There was a problem hiding this comment.
Great fixes! Thanks! The visiting times overhead can be avoided, see my suggestion :)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Revealed by clang-tidy with bugprone-use-after-move.
Also enabled some more warnings (which are already satisfied on the code base) and fixed clang-tidy scripts such that relative paths are handled correctly now.