[PATCH] D67744: [clang-tidy] Fix bugprone-argument-comment-check to correctly ignore implicit constructors.

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel created this revision.
ymandel added a reviewer: gribozavr.
Herald added a subscriber: xazax.hun.
Herald added a project: clang.

After revision 370919, this check incorrectly flags certain cases of implicit
constructors. Specifically, if an argument is annotated with an
argument-comment and the argument expression triggers an implicit constructor,
then the argument comment is associated with argument of the implicit
constructor.

However, this only happens when the constructor has more than one argument.
This revision fixes the check for implicit constructors and adds a regression
test for this case.

Note: r370919 didn't cause this bug, it simply uncovered it by fixing another
bug that was masking the behavior.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67744

Files:
  clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
  clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp


Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp
@@ -63,6 +63,28 @@
   f3(/*With_Underscores=*/false);
 }
 
+namespace IgnoresImplicit {
+struct S {
+  S(int x);
+  int x;
+};
+
+struct T {
+  // Use two arguments (one defaulted) because simplistic check for implicit
+  // constructor looks for only one argument. We need to default the argument 
so
+  // that it will still be triggered implicitly.  This is not contrived -- it
+  // comes up in real code, for example std::set(std::initializer_list...).
+  T(S s, int y = 0);
+};
+
+void k(T arg1);
+
+void mynewtest() {
+  int foo = 3;
+  k(/*arg1=*/S(foo));
+}
+} // namespace IgnoresImplicit
+
 namespace ThisEditDistanceAboveThreshold {
 void f4(int xxx);
 void g() { f4(/*xyz=*/0); }
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -337,7 +337,7 @@
                   llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()));
   } else {
     const auto *Construct = cast<CXXConstructExpr>(E);
-    if (Construct->getNumArgs() == 1 &&
+    if (Construct->getNumArgs() > 0 &&
         Construct->getArg(0)->getSourceRange() == Construct->getSourceRange()) 
{
       // Ignore implicit construction.
       return;



Index: clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp
+++ clang-tools-extra/test/clang-tidy/bugprone-argument-comment.cpp
@@ -63,6 +63,28 @@
   f3(/*With_Underscores=*/false);
 }
 
+namespace IgnoresImplicit {
+struct S {
+  S(int x);
+  int x;
+};
+
+struct T {
+  // Use two arguments (one defaulted) because simplistic check for implicit
+  // constructor looks for only one argument. We need to default the argument so
+  // that it will still be triggered implicitly.  This is not contrived -- it
+  // comes up in real code, for example std::set(std::initializer_list...).
+  T(S s, int y = 0);
+};
+
+void k(T arg1);
+
+void mynewtest() {
+  int foo = 3;
+  k(/*arg1=*/S(foo));
+}
+} // namespace IgnoresImplicit
+
 namespace ThisEditDistanceAboveThreshold {
 void f4(int xxx);
 void g() { f4(/*xyz=*/0); }
Index: clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/ArgumentCommentCheck.cpp
@@ -337,7 +337,7 @@
                   llvm::makeArrayRef(Call->getArgs(), Call->getNumArgs()));
   } else {
     const auto *Construct = cast<CXXConstructExpr>(E);
-    if (Construct->getNumArgs() == 1 &&
+    if (Construct->getNumArgs() > 0 &&
         Construct->getArg(0)->getSourceRange() == Construct->getSourceRange()) {
       // Ignore implicit construction.
       return;

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits