Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-tidy] Ignore non-forwarded arguments if they are unnamed #87832

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

SimplyDanny
Copy link
Member

Fixes #87697.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 5, 2024

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Danny Mösch (SimplyDanny)

Changes

Fixes #87697.


Full diff: https://github.com/llvm/llvm-project/pull/87832.diff

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp (+9-6)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp (+3)
diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
index 87fd8adf997082..6817bcb24c8112 100644
--- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
+++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp
@@ -79,6 +79,8 @@ AST_MATCHER_P(LambdaExpr, hasCaptureDefaultKind, LambdaCaptureDefault, Kind) {
   return Node.getCaptureDefault() == Kind;
 }
 
+AST_MATCHER(ParmVarDecl, hasAnyName) { return !Node.getName().empty(); }
+
 } // namespace
 
 void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) {
@@ -125,12 +127,13 @@ void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) {
                    hasAncestor(expr(hasUnevaluatedContext())))));
 
   Finder->addMatcher(
-      parmVarDecl(parmVarDecl().bind("param"), isTemplateTypeParameter(),
-                  hasAncestor(functionDecl().bind("func")),
-                  hasAncestor(functionDecl(
-                      isDefinition(), equalsBoundNode("func"), ToParam,
-                      unless(anyOf(isDeleted(), hasDescendant(std::move(
-                                                    ForwardCallMatcher))))))),
+      parmVarDecl(
+          parmVarDecl().bind("param"), hasAnyName(), isTemplateTypeParameter(),
+          hasAncestor(functionDecl().bind("func")),
+          hasAncestor(functionDecl(
+              isDefinition(), equalsBoundNode("func"), ToParam,
+              unless(anyOf(isDeleted(),
+                           hasDescendant(std::move(ForwardCallMatcher))))))),
       this);
 }
 
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 456e09204fa2f9..f79d3b9f362063 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -182,6 +182,10 @@ Changes in existing checks
   giving false positives for deleted functions and fix false negative when some
   parameters are forwarded, but other aren't.
 
+- Improved :doc:`cppcoreguidelines-missing-std-forward
+  <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by ignoring
+  parameters without a name (unused arguments).
+
 - Improved :doc:`cppcoreguidelines-owning-memory
   <clang-tidy/checks/cppcoreguidelines/owning-memory>` check to properly handle
   return type in lambdas and in nested functions.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp
index 9a50eabf619bd5..c435a0e023ba0b 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp
@@ -198,3 +198,6 @@ struct S {
 };
 
 } // namespace deleted_functions
+
+template<typename F>
+void unused_argument(F&&) {}

Copy link
Member

@PiotrZSL PiotrZSL left a comment

Choose a reason for hiding this comment

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

Current code is fine, wound be good ignore also unused.

unless(anyOf(isDeleted(), hasDescendant(std::move(
ForwardCallMatcher))))))),
parmVarDecl(
parmVarDecl().bind("param"), hasAnyName(), isTemplateTypeParameter(),
Copy link
Member

Choose a reason for hiding this comment

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

You should also ignore unused variables, like [[maybe_unused]] with:

unless(hasAttr(attr::Kind::Unused)),

And you could also use:

unless(isReferenced()),

with:

AST_MATCHER(VarDecl, isReferenced) { return Node.isReferenced(); }

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with unless(hasAttr(attr::Kind::Unused)) and hasIdentifier. isReferenced() doesn't play nicely with constructor initializer lists. It seems that it doesn't consider a declaration used when it's only used to initialize a field.

@SimplyDanny SimplyDanny force-pushed the ignore-unused-arguments branch 2 times, most recently from 25bc91c to 891947e Compare April 7, 2024 17:55
@SimplyDanny SimplyDanny merged commit 16b3e43 into llvm:main Apr 8, 2024
5 checks passed
@SimplyDanny SimplyDanny deleted the ignore-unused-arguments branch April 8, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cppcoreguidelines-missing-std-forward should be ignored for unused input arguments
5 participants