Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Implement package preprocessor2#898

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
MichaelRFairhurst wants to merge8 commits intomain
base:main
Choose a base branch
Loading
fromimplement-package-preprocessor2

Conversation

MichaelRFairhurst
Copy link
Contributor

Description

Implement package preprocessor2. Builds on#893

Rule 19-3-4 has a number of reports in popular repositories that are not false positives by the rule text:

  • most commonly, macros like#define M(X, Y) f(X, Y) which do not need parenthesis but the rule text disallows
  • Cases of macros calling macros,#define M1(..., X, ...) ... (X) ...,#define M2(X) M1(..., X, ...) whether this violates the rule by text is ambiguous. This is a subset of the above case. Very difficult to detect.
  • much more rarely,M(arr[1 * 1]) is non-compliant by rule text but clearly safe.MatchingParenthesis module could be extended to exclude these findings, with additional work.
  • Other rare cases, for example#define M(X) {X;}

The matching parenthesis parser is does not have error recovery. There are potentially false negatives when parenthesis are unbalanced, but

  • such cases should be rare, require trickery to even write
  • such cases should indicate a low confidence in our analysis

Adding error recovery would likely be a modest amount of work. An easier solution, though it still suffers from the same two above problems, would be a recursivegetDepth function that matches the behavior described in the rule text (depth +1 on '(', depth - 1 on ')', depth unchanged otherwise).

Change request type

  • Release or process automation (GitHub workflows, internal scripts)
  • Internal documentation
  • External documentation
  • Query files (.ql,.qll,.qls or unit tests)
  • External scripts (analysis report or other code shipped as part of a release)

Rules with added or modified queries

  • No rules added
  • Queries have been added for the following rules:
    • RULE-19-2-2
    • RULE-19-3-4
    • RULE-19-6-1
  • Queries have been modified for the following rules:
    • rule number here

Release change checklist

A change note (development_handbook.md#change-notes) is required for any pull request which modifies:

  • The structure or layout of the release artifacts.
  • The evaluation performance (memory, execution time) of an existing query.
  • The results of an existing query in any circumstance.

If you are only adding new rule queries, a change note is not required.

Author: Is a change note required?

  • Yes
  • No

🚨🚨🚨
Reviewer: Confirm that format ofshared queries (not the .qll file, the
.ql file that imports it) is valid by running them within VS Code.

  • Confirmed

Reviewer: Confirm that either a change note is not required or the change note is required and has been added.

  • Confirmed

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

  • Have all the relevant rule package description files been checked in?
  • Have you verified that the metadata properties of each new query is set appropriately?
  • Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
  • Are the alert messages properly formatted and consistent with thestyle guide?
  • Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
    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.
  • Does the query have an appropriate level of in-query comments/documentation?
  • Have you considered/identified possible edge cases?
  • Does the query not reinvent features in the standard library?
  • Can the query be simplified further (not golfed!)

Reviewer

  • Have all the relevant rule package description files been checked in?
  • Have you verified that the metadata properties of each new query is set appropriately?
  • Do all the unit tests contain both "COMPLIANT" and "NON_COMPLIANT" cases?
  • Are the alert messages properly formatted and consistent with thestyle guide?
  • Have you run the queries on OpenPilot and verified that the performance and results are acceptable?
    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.
  • Does the query have an appropriate level of in-query comments/documentation?
  • Have you considered/identified possible edge cases?
  • Does the query not reinvent features in the standard library?
  • Can the query be simplified further (not golfed!)

@CopilotCopilotAI review requested due to automatic review settingsMay 14, 2025 18:17
Copy link
Contributor

@CopilotCopilotAI 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

Adds a new “preprocessor2” package with MISRA C++ queries for include directives, macro argument parenthesis, and pragma usage, along with supporting changes to the matching‐parenthesis and macro utilities.

  • Introduce three new queries for RULE-19-2-2 (invalid#include), RULE-19-3-4 (unparenthesized macro arguments), and RULE-19-6-1 (disallowed#pragma).
  • ExtendMatchingParenthesis to handle end‐of‐input and addparameterPrecedenceUnprotected toMacro.qll.
  • Update exclusion metadata and add.qlref/.expected test definitions for the three new rules.

Reviewed Changes

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

Show a summary per file
FileDescription
cpp/misra/src/rules/RULE-19-2-2/InvalidIncludeDirective.qlNew query to enforce<file> or"file" syntax in#include
cpp/misra/src/rules/RULE-19-3-4/UnparenthesizedMacroArgument.qlNew query to flag macro arguments missing parentheses around critical operators
cpp/misra/src/rules/RULE-19-6-1/DisallowedUseOfPragma.qlNew query disallowing#pragma and_Pragma usage
cpp/common/src/codingstandards/cpp/MatchingParenthesis.qllRegex update to support end‐of‐input in parenthesis matching
cpp/common/src/codingstandards/cpp/Macro.qllAddedparameterPrecedenceUnprotected predicate
cpp/common/src/codingstandards/cpp/exclusions/cpp/Preprocessor2.qllExclusion metadata for new queries
cpp/misra/test/rules/RULE-19-1-1/*.expected & *.qlrefAdded expected outputs and refs for defined‐operator tests
Comments suppressed due to low confidence (1)

cpp/misra/src/rules/RULE-19-2-2/InvalidIncludeDirective.ql:1

  • No unit tests provided for this new query (and similarly for RULE-19-3-4 and RULE-19-6-1). Please add compliant and non-compliant test cases undercpp/misra/test/rules for full coverage.
/**

result =
".*(" +
concat(string op |
op in [
Copy link
Preview

CopilotAIMay 14, 2025

Choose a reason for hiding this comment

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

The regex built incriticalOperatorRegex concatenates a trailing '|' inside the capture group, introducing an empty alternative that matches any input and causinghasCriticalOperator to always return true. Remove the trailing '|' or build the pattern without leading/trailing separators.

Copilot uses AI. Check for mistakes.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Pretty sure this is incorrect, asconcat(... | v, "|") should function likejoin() and not add a trailing|.

@@ -61,7 +61,7 @@ module MatchingParenthesis<InputString Input> {
occurrence = prevOccurrence + 1
) else (
token = TNotParen() and
exists(inputStr.regexpFind("\\(|\\)", prevOccurrence + 1, endPos)) and
exists(inputStr.regexpFind("\\(|\\)|$", prevOccurrence + 1, endPos)) and
Copy link
Preview

CopilotAIMay 14, 2025

Choose a reason for hiding this comment

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

Including '$' in the regex"\\(|\\)|$" allows matching an empty string at end‐of‐input, which can lead to zero‐length matches and infinite loops. Consider matching only parentheses here and handling end‐of‐input separately.

Suggested change
exists(inputStr.regexpFind("\\(|\\)|$",prevOccurrence+1, endPos))and
exists(inputStr.regexpFind("\\(|\\)",prevOccurrence+1, endPos))and
(endPos<inputStr.length()or endPos= inputStr.length())and

Copilot uses AI. Check for mistakes.

@MichaelRFairhurst
Copy link
ContributorAuthor

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

Copilot code reviewCopilotCopilot left review comments

@lcarteylcarteyAwaiting requested review from lcartey

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

1 participant
@MichaelRFairhurst

[8]ページ先頭

©2009-2025 Movatter.jp