[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG9343ec861a2e: [clang-tidy] Adding the missing handling of 
noreturn attributes for Obj-C… (authored by ziqingluo-90).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+// no warning, function term has C noreturn attribute
+term();
+  }
+  while (i < 10) {
+// no warning, class method baz has noreturn attribute
+[I baz];
+  }
+  while (i + j < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+[I foo];
+  }
+  while (i + j < 10) {
+[I foo];
+[I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+block();
+  }
+  while (i < 10) {
+// no warning, the block has "noreturn" arribute
+block_nr();
+  }
+}
+
+@implementation I
++ (void)bar {
+}
+
++ (void)foo {
+  static int i = 0;
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+[I bar];
+  }
+}
+@end
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -16,15 +16,35 @@
 using clang::tidy::utils::hasPtrOrReferenceInFunc;
 
 namespace clang {
+namespace ast_matchers {
+/// matches a Decl if it has a  "no return" attribute of any kind
+AST_MATCHER(Decl, declHasNoReturnAttr) {
+  return Node.hasAttr() || Node.hasAttr() ||
+ Node.hasAttr();
+}
+
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();
+}
+} // namespace ast_matchers
 namespace tidy {
 namespace bugprone {
 
 static internal::Matcher
 loopEndingStmt(internal::Matcher Internal) {
-  // FIXME: Cover noreturn ObjC methods (and blocks?).
+  internal::Matcher isNoReturnFunType =
+  ignoringParens(functionType(typeHasNoReturnAttr()));
+  internal::Matcher isNoReturnDecl =
+  anyOf(declHasNoReturnAttr(), functionDecl(hasType(isNoReturnFunType)),
+varDecl(hasType(blockPointerType(pointee(isNoReturnFunType);
+
   return stmt(anyOf(
   mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
-  callExpr(Internal, callee(functionDecl(isNoReturn());
+  callExpr(Internal,
+   callee(mapAnyOf(functionDecl, /* block callee */ varDecl)
+  .with(isNoReturnDecl))),
+  objcMessageExpr(Internal, callee(isNoReturnDecl;
 }
 
 /// Return whether `Var` was changed in `LoopStmt`.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Still looks good to me, let's commit!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 447854.
ziqingluo-90 added a comment.

The `callee` ASTMatcher overloading patch has landed in LLVM repo.  I update 
this patch to use `callee` for matching objective-C message callee methods.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+// no warning, function term has C noreturn attribute
+term();
+  }
+  while (i < 10) {
+// no warning, class method baz has noreturn attribute
+[I baz];
+  }
+  while (i + j < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+[I foo];
+  }
+  while (i + j < 10) {
+[I foo];
+[I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+block();
+  }
+  while (i < 10) {
+// no warning, the block has "noreturn" arribute
+block_nr();
+  }
+}
+
+@implementation I
++ (void)bar {
+}
+
++ (void)foo {
+  static int i = 0;
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+[I bar];
+  }
+}
+@end
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -16,15 +16,35 @@
 using clang::tidy::utils::hasPtrOrReferenceInFunc;
 
 namespace clang {
+namespace ast_matchers {
+/// matches a Decl if it has a  "no return" attribute of any kind
+AST_MATCHER(Decl, declHasNoReturnAttr) {
+  return Node.hasAttr() || Node.hasAttr() ||
+ Node.hasAttr();
+}
+
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();
+}
+} // namespace ast_matchers
 namespace tidy {
 namespace bugprone {
 
 static internal::Matcher
 loopEndingStmt(internal::Matcher Internal) {
-  // FIXME: Cover noreturn ObjC methods (and blocks?).
+  internal::Matcher isNoReturnFunType =
+  ignoringParens(functionType(typeHasNoReturnAttr()));
+  internal::Matcher isNoReturnDecl =
+  anyOf(declHasNoReturnAttr(), functionDecl(hasType(isNoReturnFunType)),
+varDecl(hasType(blockPointerType(pointee(isNoReturnFunType);
+
   return stmt(anyOf(
   mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
-  callExpr(Internal, callee(functionDecl(isNoReturn());
+  callExpr(Internal,
+   callee(mapAnyOf(functionDecl, /* block callee */ varDecl)
+  .with(isNoReturnDecl))),
+  objcMessageExpr(Internal, callee(isNoReturnDecl;
 }
 
 /// Return whether `Var` was changed in `LoopStmt`.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ accepted this revision.
NoQ added a comment.

Thanks! Good to go after the matchers patch is committed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added a comment.

In D128314#3635861 , @njames93 wrote:

> Sorry to do this again, but could this be split up again, one patch for the 
> new matcher and the tests associated with it, then another for the actual bug 
> fix.
> Also cc @klimek as he is the code owner of ASTMatchers

Makes sense.  I have created a new review  request 
 for the ASTMatcher.   This patch now depends 
on it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Nathan James via Phabricator via cfe-commits]

njames93 added a subscriber: klimek.
njames93 added a comment.

Sorry to do this again, but could this be split up again, one patch for the new 
matcher and the tests associated with it, then another for the actual bug fix.
Also cc @klimek as he is the code owner of ASTMatchers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Sounds great! I have a nitpick but other than that I think this fix is good to 
go.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:3841
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///

It looks like documentation traditionally starts with a capital letter and ends 
with `.`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 added a comment.

In D128314#3605588 , @NoQ wrote:

> Nice!
>
> There's usually some bureaucracy when creating new matchers, i.e. there 
> should be documentation and unittests covering them.

I have added a unit test and auto-generated document for it.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5099
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();

NoQ wrote:
> Can we make a polymorphic matcher `hasNoReturnAttr` that can accept either 
> decl or type?
I've moved these two matchers, which seems are specific to the infinite loop 
checker, to `InfiniteLoopCheck.cpp`.  Maybe they no longer need to be 
generalized by polymorphism. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 441832.
ziqingluo-90 added a comment.

adding a unit test for the ASTMatcher `objcMessageCallee` added in ASTMatcher.h


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp
  clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

Index: clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
===
--- clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
+++ clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp
@@ -2352,6 +2352,37 @@
argumentCountIs(0;
 }
 
+TEST(ASTMatchersTestObjC, ObjCMessageCallee) {
+  //  Will NOT match function callee:
+  EXPECT_TRUE(notMatchesObjC("void f() {"
+ "  f(); "
+ "}",
+ objcMessageExpr(objcMessageCallee(hasName("f");
+
+  StringRef Objc1String = "@interface I "
+  " - (void)instanceMethod;"
+  " + (void)classMethod;"
+  " + (void)uncalledMethod;"
+  "@end\n"
+  "int main(void) {\n"
+  "  [I classMethod];"
+  "  I *i = [[I alloc] init];"
+  "  [i instanceMethod];"
+  "}";
+  // Should find the two method declarations through the message expressions:
+  EXPECT_TRUE(
+  matchesObjC(Objc1String, objcMessageExpr(objcMessageCallee(
+   objcMethodDecl(hasName("classMethod"));
+  EXPECT_TRUE(matchesObjC(Objc1String,
+  objcMessageExpr(objcMessageCallee(
+  objcMethodDecl(hasName("instanceMethod"));
+  // Will NOT match a method declaration through `objcMessageCallee` if there is
+  // no such message expression:
+  EXPECT_FALSE(matchesObjC(Objc1String,
+   objcMessageExpr(objcMessageCallee(
+   objcMethodDecl(hasName("uncalledMethod"));
+}
+
 TEST(ASTMatchersTestObjC, ObjCStringLiteral) {
 
   StringRef Objc1String = "@interface NSObject "
Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -501,6 +501,7 @@
   REGISTER_MATCHER(objcIvarDecl);
   REGISTER_MATCHER(objcIvarRefExpr);
   REGISTER_MATCHER(objcMessageExpr);
+  REGISTER_MATCHER(objcMessageCallee);
   REGISTER_MATCHER(objcMethodDecl);
   REGISTER_MATCHER(objcObjectPointerType);
   REGISTER_MATCHER(objcPropertyDecl);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3838,6 +3838,25 @@
   InnerMatcher.matches(*ExprNode, Finder, Builder));
 }
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///
+/// Given
+/// \code
+///   @interface I: NSObject
+///   +(void)foo;
+///   @end
+///   ...
+///   [I foo]
+/// \endcode
+/// The example above matches \code [I foo] \endcode with
+/// objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+AST_MATCHER_P(ObjCMessageExpr, objcMessageCallee,
+  internal::Matcher, InnerMatcher) {
+  const ObjCMethodDecl *msgDecl = Node.getMethodDecl();
+  return (msgDecl != nullptr &&
+  InnerMatcher.matches(*msgDecl, Finder, Builder));
+}
+
 /// Matches if the call expression's callee's declaration matches the
 /// given matcher.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -8867,6 +8867,20 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMessageExpr.html;>ObjCMessageExprobjcMessageCalleeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html;>ObjCMethodDecl InnerMatcher
+matches if ObjCMessageExpr's callee declaration matches
+
+Given
+  @interface I: NSObject
+  +(void)foo;
+  @end
+  ...
+  [I foo]
+The example above matches [I foo] with
+objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html;>ObjCMethodDeclhasAnyParameterMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ParmVarDecl.html;>ParmVarDecl InnerMatcher
 Matches any parameter of a function or an ObjC method 

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 441816.
ziqingluo-90 added a comment.

I saw @aaron.ballman 's comment "//We typically don't add AST matchers until we 
have a need for them to be used in-tree (ASTMatchers.h is already really 
expensive to parse; adding matchers for everything possible with AST nodes 
would be prohibitively expensive).//" in this patch 
.   So I think the matchers for testing 
`noreturn` attributes is too specific to be in the `ASTMatcher.h`.  Moving them 
to where they are used.   While the `objcMessageCallee` is sort of the Obj-C 
counterpart of the `callee` matcher, so I think it is general enough to stay.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
  clang/docs/LibASTMatchersReference.html
  clang/include/clang/ASTMatchers/ASTMatchers.h
  clang/lib/ASTMatchers/Dynamic/Registry.cpp

Index: clang/lib/ASTMatchers/Dynamic/Registry.cpp
===
--- clang/lib/ASTMatchers/Dynamic/Registry.cpp
+++ clang/lib/ASTMatchers/Dynamic/Registry.cpp
@@ -501,6 +501,7 @@
   REGISTER_MATCHER(objcIvarDecl);
   REGISTER_MATCHER(objcIvarRefExpr);
   REGISTER_MATCHER(objcMessageExpr);
+  REGISTER_MATCHER(objcMessageCallee);
   REGISTER_MATCHER(objcMethodDecl);
   REGISTER_MATCHER(objcObjectPointerType);
   REGISTER_MATCHER(objcPropertyDecl);
Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3838,6 +3838,25 @@
   InnerMatcher.matches(*ExprNode, Finder, Builder));
 }
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///
+/// Given
+/// \code
+///   @interface I: NSObject
+///   +(void)foo;
+///   @end
+///   ...
+///   [I foo]
+/// \endcode
+/// The example above matches \code [I foo] \endcode with
+/// objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+AST_MATCHER_P(ObjCMessageExpr, objcMessageCallee,
+  internal::Matcher, InnerMatcher) {
+  const ObjCMethodDecl *msgDecl = Node.getMethodDecl();
+  return (msgDecl != nullptr &&
+  InnerMatcher.matches(*msgDecl, Finder, Builder));
+}
+
 /// Matches if the call expression's callee's declaration matches the
 /// given matcher.
 ///
Index: clang/docs/LibASTMatchersReference.html
===
--- clang/docs/LibASTMatchersReference.html
+++ clang/docs/LibASTMatchersReference.html
@@ -8867,6 +8867,20 @@
 
 
 
+Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMessageExpr.html;>ObjCMessageExprobjcMessageCalleeMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html;>ObjCMethodDecl InnerMatcher
+matches if ObjCMessageExpr's callee declaration matches
+
+Given
+  @interface I: NSObject
+  +(void)foo;
+  @end
+  ...
+  [I foo]
+The example above matches [I foo] with
+objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+
+
+
 Matcherhttps://clang.llvm.org/doxygen/classclang_1_1ObjCMethodDecl.html;>ObjCMethodDeclhasAnyParameterMatcherhttps://clang.llvm.org/doxygen/classclang_1_1ParmVarDecl.html;>ParmVarDecl InnerMatcher
 Matches any parameter of a function or an ObjC method declaration or a
 block.
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+// no warning, function term has C noreturn attribute
+term();
+  }
+  while (i < 10) {
+// no warning, class method baz has noreturn attribute
+[I baz];
+  }
+  while (i + j < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+[I foo];
+  }
+  while (i + j < 10) {
+[I foo];
+[I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none 

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 439549.
ziqingluo-90 added a comment.

adjusted my changes with respect to the recent file structure changes in 
clang-tidy test


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
  clang/include/clang/ASTMatchers/ASTMatchers.h

Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3829,6 +3829,25 @@
   InnerMatcher.matches(*ExprNode, Finder, Builder));
 }
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///
+/// Given
+/// \code
+///   @interface I: NSObject
+///   +(void)foo;
+///   @end
+///   ...
+///   [I foo]
+/// \endcode
+/// The example above matches \code [I foo] \endcode with
+/// objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+AST_MATCHER_P(ObjCMessageExpr, objcMessageCallee,
+  internal::Matcher, InnerMatcher) {
+  const ObjCMethodDecl *msgDecl = Node.getMethodDecl();
+  return (msgDecl != nullptr &&
+  InnerMatcher.matches(*msgDecl, Finder, Builder));
+}
+
 /// Matches if the call expression's callee's declaration matches the
 /// given matcher.
 ///
@@ -5070,6 +5089,17 @@
 /// \endcode
 AST_MATCHER(FunctionDecl, isNoReturn) { return Node.isNoReturn(); }
 
+/// matches a Decl if it has a  "no return" attribute of any kind
+AST_MATCHER(Decl, declHasNoReturnAttr) {
+  return Node.hasAttr() || Node.hasAttr() ||
+ Node.hasAttr();
+}
+
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();
+}
+
 /// Matches the return type of a function declaration.
 ///
 /// Given:
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/infinite-loop-noreturn.mm
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+// no warning, function term has C noreturn attribute
+term();
+  }
+  while (i < 10) {
+// no warning, class method baz has noreturn attribute
+[I baz];
+  }
+  while (i + j < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+[I foo];
+  }
+  while (i + j < 10) {
+[I foo];
+[I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+block();
+  }
+  while (i < 10) {
+// no warning, the block has "noreturn" arribute
+block_nr();
+  }
+}
+
+@implementation I
++ (void)bar {
+}
+
++ (void)foo {
+  static int i = 0;
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+[I bar];
+  }
+}
+@end
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -21,10 +21,17 @@
 
 static internal::Matcher
 loopEndingStmt(internal::Matcher Internal) {
-  // FIXME: Cover noreturn ObjC methods (and blocks?).
+  internal::Matcher isNoReturnFunType =
+  ignoringParens(functionType(typeHasNoReturnAttr()));
+  internal::Matcher isNoReturnDecl =
+  anyOf(declHasNoReturnAttr(), functionDecl(hasType(isNoReturnFunType)),
+varDecl(hasType(blockPointerType(pointee(isNoReturnFunType);
+
   return stmt(anyOf(
   mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
-  callExpr(Internal, callee(functionDecl(isNoReturn());
+  callExpr(Internal, callee(functionDecl(isNoReturnDecl))),
+  objcMessageExpr(Internal, objcMessageCallee(isNoReturnDecl)),
+  callExpr(Internal, callee(varDecl(isNoReturnDecl);
 }
 
 

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 439545.
ziqingluo-90 removed reviewers: rsundahl, yln, kubamracek, krispy1994, jkorous, 
delcypher, chrisdangelo, thetruestblue, dcoughlin.
ziqingluo-90 added a comment.

rebased with the latest main's HEAD


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop-noreturn.mm
  clang/include/clang/ASTMatchers/ASTMatchers.h

Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3829,6 +3829,25 @@
   InnerMatcher.matches(*ExprNode, Finder, Builder));
 }
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///
+/// Given
+/// \code
+///   @interface I: NSObject
+///   +(void)foo;
+///   @end
+///   ...
+///   [I foo]
+/// \endcode
+/// The example above matches \code [I foo] \endcode with
+/// objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+AST_MATCHER_P(ObjCMessageExpr, objcMessageCallee,
+  internal::Matcher, InnerMatcher) {
+  const ObjCMethodDecl *msgDecl = Node.getMethodDecl();
+  return (msgDecl != nullptr &&
+  InnerMatcher.matches(*msgDecl, Finder, Builder));
+}
+
 /// Matches if the call expression's callee's declaration matches the
 /// given matcher.
 ///
@@ -5070,6 +5089,17 @@
 /// \endcode
 AST_MATCHER(FunctionDecl, isNoReturn) { return Node.isNoReturn(); }
 
+/// matches a Decl if it has a  "no return" attribute of any kind
+AST_MATCHER(Decl, declHasNoReturnAttr) {
+  return Node.hasAttr() || Node.hasAttr() ||
+ Node.hasAttr();
+}
+
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();
+}
+
 /// Matches the return type of a function declaration.
 ///
 /// Given:
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop-noreturn.mm
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop-noreturn.mm
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+// no warning, function term has C noreturn attribute
+term();
+  }
+  while (i < 10) {
+// no warning, class method baz has noreturn attribute
+[I baz];
+  }
+  while (i + j < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+[I foo];
+  }
+  while (i + j < 10) {
+[I foo];
+[I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+block();
+  }
+  while (i < 10) {
+// no warning, the block has "noreturn" arribute
+block_nr();
+  }
+}
+
+@implementation I
++ (void)bar {
+}
+
++ (void)foo {
+  static int i = 0;
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+[I bar];
+  }
+}
+@end
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -21,10 +21,17 @@
 
 static internal::Matcher
 loopEndingStmt(internal::Matcher Internal) {
-  // FIXME: Cover noreturn ObjC methods (and blocks?).
+  internal::Matcher isNoReturnFunType =
+  ignoringParens(functionType(typeHasNoReturnAttr()));
+  internal::Matcher isNoReturnDecl =
+  anyOf(declHasNoReturnAttr(), functionDecl(hasType(isNoReturnFunType)),
+varDecl(hasType(blockPointerType(pointee(isNoReturnFunType);
+
   return stmt(anyOf(
   mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
-  callExpr(Internal, callee(functionDecl(isNoReturn());
+  callExpr(Internal, callee(functionDecl(isNoReturnDecl))),
+  objcMessageExpr(Internal, 

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto `main:HEAD` and:

- fold your changes into the appropriate subdirs, stripping the module prefix 
from new files.
- make the target `check-clang-extra` to validate your tests
- make the target `docs-clang-tools-html` to validate any docs changes


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Nice!

There's usually some bureaucracy when creating new matchers, i.e. there should 
be documentation and unittests covering them.




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:5099
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();

Can we make a polymorphic matcher `hasNoReturnAttr` that can accept either decl 
or type?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

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

[PATCH] D128314: [Clang-tidy] Fixing a bug in clang-tidy infinite-loop checker

[Ziqing Luo via Phabricator via cfe-commits]

ziqingluo-90 updated this revision to Diff 439132.
ziqingluo-90 retitled this revision from "[Clang-tidy] Fixing bugs in 
clang-tidy infinite-loop checker" to "[Clang-tidy] Fixing a bug in clang-tidy 
infinite-loop checker".
ziqingluo-90 edited the summary of this revision.
ziqingluo-90 added a comment.

Separate the change adding handling of noreturn attributes for ObjC nodes.  
Have run clang-format.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128314/new/

https://reviews.llvm.org/D128314

Files:
  clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
  clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop-noreturn.mm
  clang/include/clang/ASTMatchers/ASTMatchers.h

Index: clang/include/clang/ASTMatchers/ASTMatchers.h
===
--- clang/include/clang/ASTMatchers/ASTMatchers.h
+++ clang/include/clang/ASTMatchers/ASTMatchers.h
@@ -3829,6 +3829,25 @@
   InnerMatcher.matches(*ExprNode, Finder, Builder));
 }
 
+/// matches if ObjCMessageExpr's callee declaration matches
+///
+/// Given
+/// \code
+///   @interface I: NSObject
+///   +(void)foo;
+///   @end
+///   ...
+///   [I foo]
+/// \endcode
+/// The example above matches \code [I foo] \endcode with
+/// objcMessageExpr(objcMessageCallee(objcMethodDecl(hasName("foo"
+AST_MATCHER_P(ObjCMessageExpr, objcMessageCallee,
+  internal::Matcher, InnerMatcher) {
+  const ObjCMethodDecl *msgDecl = Node.getMethodDecl();
+  return (msgDecl != nullptr &&
+  InnerMatcher.matches(*msgDecl, Finder, Builder));
+}
+
 /// Matches if the call expression's callee's declaration matches the
 /// given matcher.
 ///
@@ -5070,6 +5089,17 @@
 /// \endcode
 AST_MATCHER(FunctionDecl, isNoReturn) { return Node.isNoReturn(); }
 
+/// matches a Decl if it has a  "no return" attribute of any kind
+AST_MATCHER(Decl, declHasNoReturnAttr) {
+  return Node.hasAttr() || Node.hasAttr() ||
+ Node.hasAttr();
+}
+
+/// matches a FunctionType if the type includes the GNU no return attribute
+AST_MATCHER(FunctionType, typeHasNoReturnAttr) {
+  return Node.getNoReturnAttr();
+}
+
 /// Matches the return type of a function declaration.
 ///
 /// Given:
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop-noreturn.mm
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-infinite-loop-noreturn.mm
@@ -0,0 +1,62 @@
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fexceptions
+// RUN: %check_clang_tidy %s bugprone-infinite-loop %t -- -- -fblocks -fobjc-arc -fexceptions
+
+@interface I
++ (void)foo;
++ (void)bar;
++ (void)baz __attribute__((noreturn));
++ (instancetype)alloc;
+- (instancetype)init;
+@end
+
+_Noreturn void term();
+
+void plainCFunction() {
+  int i = 0;
+  int j = 0;
+  int a[10];
+
+  while (i < 10) {
+// no warning, function term has C noreturn attribute
+term();
+  }
+  while (i < 10) {
+// no warning, class method baz has noreturn attribute
+[I baz];
+  }
+  while (i + j < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i, j) are updated in the loop body [bugprone-infinite-loop]
+[I foo];
+  }
+  while (i + j < 10) {
+[I foo];
+[I baz]; // no warning, class method baz has noreturn attribute
+  }
+
+  void (^block)() = ^{
+  };
+  void __attribute__((noreturn)) (^block_nr)(void) = ^void __attribute__((noreturn)) (void) { throw "err"; };
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+block();
+  }
+  while (i < 10) {
+// no warning, the block has "noreturn" arribute
+block_nr();
+  }
+}
+
+@implementation I
++ (void)bar {
+}
+
++ (void)foo {
+  static int i = 0;
+
+  while (i < 10) {
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: this loop is infinite; none of its condition variables (i) are updated in the loop body [bugprone-infinite-loop]
+[I bar];
+  }
+}
+@end
Index: clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
===
--- clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
+++ clang-tools-extra/clang-tidy/bugprone/InfiniteLoopCheck.cpp
@@ -21,10 +21,17 @@
 
 static internal::Matcher
 loopEndingStmt(internal::Matcher Internal) {
-  // FIXME: Cover noreturn ObjC methods (and blocks?).
+  internal::Matcher isNoReturnFunType =
+  ignoringParens(functionType(typeHasNoReturnAttr()));
+  internal::Matcher isNoReturnDecl =
+  anyOf(declHasNoReturnAttr(), functionDecl(hasType(isNoReturnFunType)),
+varDecl(hasType(blockPointerType(pointee(isNoReturnFunType);
+
   return stmt(anyOf(
   mapAnyOf(breakStmt, returnStmt, gotoStmt, cxxThrowExpr).with(Internal),
-  callExpr(Internal,