Skip to content

Use cppcheck right #654

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
AlexInLog merged 9 commits into from
Oct 8, 2024
Merged

Use cppcheck right #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ea64b22
use cppcheck right
AlexInLog Oct 7, 2024
790b5b2
fix
AlexInLog Oct 7, 2024
c4760f2
change way
AlexInLog Oct 7, 2024
1ec1d93
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 7, 2024
1025bff
fix
AlexInLog Oct 7, 2024
64a760e
suppress
AlexInLog Oct 7, 2024
25e4183
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 7, 2024
aa955b8
fix
AlexInLog Oct 7, 2024
e75e4e9
Update cppcheck.supp
AlexInLog Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,14 @@
"name": "cppcheck",
"hidden": true,
"cacheVariables": {
"CMAKE_CXX_CPPCHECK": "cppcheck;--inline-suppr;--std=c++20;--enable=all;--check-config;--suppress=missingIncludeSystem"
"CMAKE_CXX_CPPCHECK": "cppcheck;--inline-suppr;--std=c++20;--enable=all;--suppressions-list=${sourceDir}/cppcheck.supp"
}
},
{
"name": "clang-tidy",
"hidden": true,
"cacheVariables": {
"CMAKE_CXX_CLANG_TIDY": "clang-tidy;-header-filter=${sourceDir}/src/.*;-checks=-*,bugprone-*,concurrency-*,performance-*,-macro*,-bugprone-exception-escape;-warnings-as-errors=*"
"CMAKE_CXX_CLANG_TIDY": "clang-tidy;-header-filter=${sourceDir}/src/.*;-checks=-*,bugprone-*,concurrency-*,performance-*,-macro*,-bugprone-exception-escape;-warnings-as-errors=*;-extra-arg=-std=c++20"
}
},
{
Expand Down
13 changes: 13 additions & 0 deletions cppcheck.supp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
missingIncludeSystem

noExplicitConstructor

unknownMacro

shadowFunction

unusedVariable

localMutex
Comment on lines +1 to +11

@coderabbitai coderabbitai Bot Oct 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the implications of using these suppression rules and document their rationale.

The addition of these suppression rules for Cppcheck is noted. While suppression rules can be useful for managing false positives or known issues, they should be used judiciously to avoid hiding potential problems. Here are some considerations:

  1. missingIncludeSystem: Generally safe, but ensure all necessary system headers are available on the target system.
  2. noExplicitConstructor: Be cautious as this might hide issues with implicit conversions.
  3. unknownMacro: Ensure all macros are properly defined to avoid suppressing genuine issues.
  4. shadowFunction: Use carefully as function shadowing can lead to confusion and bugs.
  5. unusedVariable: Monitor to prevent code clutter.
  6. localMutex: Be aware that this might hide potential threading issues.

Consider the following recommendations:

  1. Document the specific reasons for each suppression rule, explaining why it's necessary and any associated risks.
  2. Implement a process for periodic review of these suppressions to ensure they remain relevant and necessary.
  3. If possible, address the underlying issues that necessitate these suppressions in the long term.

Example documentation format:

# Cppcheck Suppression Rules

1. missingIncludeSystem
   Reason: [Explain why this suppression is necessary]
   Risk: [Describe any potential risks associated with this suppression]
   Review Date: [Date for next review]

[Repeat for each rule]

This documentation will help maintain transparency about the suppressed warnings and facilitate future code maintenance.


unmatchedSuppression

@coderabbitai coderabbitai Bot Oct 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the implications of suppressing unmatched suppression warnings.

The unmatchedSuppression rule suppresses warnings about suppression rules that don't match any warnings. While this can reduce noise in the Cppcheck output, it may hide important information:

  1. It could mask outdated suppression rules that are no longer needed, leading to unnecessary complexity in your suppression file.
  2. It might hide incorrectly specified suppression rules, potentially leaving real issues unsuppressed.

Consider implementing a process to periodically review all suppression rules, especially focusing on those that don't match any warnings. This will help maintain a clean and effective set of suppression rules.

You could add a comment in this file to remind about periodic reviews:

# unmatchedSuppression
# Note: Periodically review all suppression rules, especially those that don't match any warnings.
# Next review date: [specify a date]
unmatchedSuppression

Comment on lines +1 to +13

@coderabbitai coderabbitai Bot Oct 8, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance file structure and documentation for better maintainability.

While the individual suppression rules have been addressed, the overall structure and documentation of this file could be improved:

  1. Add a header comment explaining the purpose of this file and how to use it.
  2. Group related suppressions together.
  3. Add comments for each suppression rule explaining its purpose and potential risks.

Consider restructuring the file as follows:

# Cppcheck Suppression File
# This file contains suppression rules for Cppcheck static analysis tool.
# Each rule should be reviewed periodically to ensure it's still necessary.

# System and compiler-related suppressions
missingIncludeSystem  # Suppresses warnings about missing system includes
unknownMacro  # Suppresses warnings about unknown macros

# Code style and design suppressions
noExplicitConstructor  # Suppresses warnings about non-explicit constructors
shadowFunction  # Suppresses warnings about function shadowing
unusedVariable  # Suppresses warnings about unused variables

# Threading-related suppressions
localMutex  # Suppresses warnings related to local mutexes

# Meta-suppressions
unmatchedSuppression  # Suppresses warnings about unmatched suppressions

# Next review date: [specify a date]

This structure provides context for each suppression and groups related rules together, making the file easier to maintain and understand.

2 changes: 1 addition & 1 deletion src/rpp/rpp/observers/observer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ namespace rpp
return dynamic_observer<Type>{std::move(*this)};
}

const dynamic_observer<Type>& as_dynamic() &
dynamic_observer<Type> as_dynamic() &
{
return dynamic_observer<Type>{*this};
}
Expand Down