Skip to content

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

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

Merged
merged 5 commits into from
Apr 8, 2024

Conversation

SimplyDanny
Copy link
Member

Fixes #87697.

@llvmbot
Copy link
Member

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&&) {}

@SimplyDanny SimplyDanny requested a review from PiotrZSL April 6, 2024 13:41
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

@SimplyDanny SimplyDanny Apr 7, 2024

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 force-pushed the ignore-unused-arguments branch from 891947e to 0cb8672 Compare April 8, 2024 17:55
@SimplyDanny SimplyDanny merged commit 16b3e43 into llvm:main Apr 8, 2024
@SimplyDanny SimplyDanny deleted the ignore-unused-arguments branch April 8, 2024 21:54
5chmidti added a commit to 5chmidti/llvm-project that referenced this pull request Aug 31, 2024
Since llvm#87832, unnamed identifiers are excluded from being diagnosed. As
a result, the tests that were supposed to test that deleted functions
are correctly ignored are ignored because of the unnamed identifiers
instead of the deleted function. This change simply introduces names for
the parameters of the deleted functions.
5chmidti added a commit that referenced this pull request Sep 11, 2024
…ard (#106861)

Since #87832, unnamed identifiers are excluded from being diagnosed. As
a result, the tests that were supposed to test that deleted functions
are correctly ignored, are ignored because of the unnamed identifiers
instead of the deleted function. This change simply introduces names for
the parameters of the deleted functions.
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