Michaelrfairhurst/sideffects 4 package 4 6 1#1049
Open
MichaelRFairhurst wants to merge 10 commits intomainfrom
Open
Michaelrfairhurst/sideffects 4 package 4 6 1#1049MichaelRFairhurst wants to merge 10 commits intomainfrom
MichaelRFairhurst wants to merge 10 commits intomainfrom
Conversation
Allows for code sharing between C and C++ even if the sequencing is different. Initial implementation of split between c++14 and c++17 sequencing.
Split configs for C++14 and C++17 sequencing. Fix bug in reciprocity.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the ordering/sequencing analysis infrastructure to use parametric modules instead of class-based configurations, and adds support for MISRA C++ 2023 RULE-4-6-1 which checks for appropriately sequenced operations on memory locations. The refactoring enables cleaner separation between C++14 and C++17 sequencing semantics.
Changes:
- Refactors
Ordering.qllto use signature modules (ConfigSig) and parametric modules (Make<Config>) for sequencing edge definitions - Adds MISRA C++ 2023 RULE-4-6-1 query for detecting unsequenced operations on memory locations (C++17 semantics)
- Updates AUTOSAR A5-0-1 and C MISRA RULE-13-2 queries to use the new parametric module pattern
- Introduces shared
ExpressionWithUnsequencedSideEffectsquery module with configurable sequencing logic
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rules.csv | Updates RULE-4-6-1 to use SideEffects4 package |
| rule_packages/cpp/SideEffects4.json | Adds new package definition for RULE-4-6-1 |
| rule_packages/cpp/OrderOfEvaluation.json | Adds shared implementation reference for A5-0-1 |
| cpp/misra/src/rules/RULE-4-6-1/MemoryUsageNotSequenced.ql | Implements new query using C++17 sequencing rules |
| cpp/misra/test/rules/RULE-4-6-1/MemoryUsageNotSequenced.testref | Links to shared test file |
| cpp/common/test/rules/expressionwithunsequencedsideeffects/test.cpp | Adds test cases with C++14/C++17 annotations |
| cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffects.ql | C++14 test query |
| cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffectsCpp17.ql | C++17 test query |
| cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffects.expected | Expected results for C++14 |
| cpp/common/test/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffectsCpp17.expected | Expected results for C++17 |
| cpp/common/src/codingstandards/cpp/rules/expressionwithunsequencedsideeffects/ExpressionWithUnsequencedSideEffects.qll | Shared query implementation with parametric module |
| cpp/common/src/codingstandards/cpp/orderofevaluation/VariableAccessOrdering.qll | Splits into Cpp14 and Cpp17 configuration modules |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/SideEffects4.qll | Adds exclusion metadata for new package |
| cpp/common/src/codingstandards/cpp/exclusions/cpp/RuleMetadata.qll | Registers SideEffects4 package |
| cpp/common/src/codingstandards/cpp/Ordering.qll | Major refactoring to use signature/parametric modules, adds cpp14Edge and cpp17Edge predicates |
| cpp/autosar/src/rules/A5-0-1/ExpressionShouldNotRelyOnOrderOfEvaluation.ql | Refactored to use new parametric module pattern |
| c/misra/src/rules/RULE-13-2/UnsequencedSideEffects.ql | Updated to use parametric module pattern |
| c/misra/src/rules/RULE-13-2/UnsequencedAtomicReads.ql | Updated to use parametric module pattern |
| c/common/src/codingstandards/c/orderofevaluation/VariableAccessOrdering.qll | Converted to use signature module |
| c/common/src/codingstandards/c/Ordering.qll | Refactored to use signature/parametric modules pattern |
Comments suppressed due to low confidence (1)
cpp/common/src/codingstandards/cpp/Ordering.qll:185
- The comment states "The right operand is sequenced before the left operand" but the code sequences lhs (n1) before rhs (n2). According to C++17 [expr.ass], the right-hand side should be evaluated before the left-hand side. The assignments should be swapped: rhs should map to n1 and lhs should map to n2.
// [expr.ass] The right operand is sequenced before the left operand for all assignment operators.
exists(Assignment assign, ConstituentExpr lhs, ConstituentExpr rhs |
lhs = assign.getLValue() and
rhs = assign.getRValue()
|
lhs = n1.toExpr().getParent*() and rhs = n2.toExpr().getParent*()
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Description
Uses parametric modules more heavily in ordering configs.
Change request type
.ql,.qll,.qlsor unit tests)Rules with added or modified queries
RULE-4-6-1A5-0-1RULE-13-2EXP50-CPPRelease change checklist
A change note (development_handbook.md#change-notes) is required for any pull request which modifies:
If you are only adding new rule queries, a change note is not required.
Author: Is a change note required?
🚨🚨🚨
Reviewer: Confirm that format of shared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.
Reviewer: Confirm that either a change note is not required or the change note is required and has been added.
Query development review checklist
For PRs that add new queries or modify existing queries, the following checklist should be completed by both the author and reviewer:
Author
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.
Reviewer
As a rule of thumb, predicates specific to the query should take no more than 1 minute, and for simple queries be under 10 seconds. If this is not the case, this should be highlighted and agreed in the code review process.