[PATCH] D19201: [clang-tidy] misc-throw-with-noexcept

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 1 06:41:16 PDT 2016


aaron.ballman requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:25
@@ +24,3 @@
+  Finder->addMatcher(
+      cxxThrowExpr(stmt(hasAncestor(functionDecl(isNoThrow()).bind("func"))))
+          .bind("throw"),
----------------
Prazek wrote:
> you can use forFunction instead of hasAncestor(functionDecl(
> cxxThrowExpr(stmt(forFunction(isNoThrow() or cxxThrowExpr(stmt(forFunction(functionDecl(isNoThrow())))
> you can even try to remove stmt.
> 
> 
> Also add test case
> 
>   void getFunction() noexcept {
>    struct X { 
>     static void inner()
>     {
>         throw 42;
>     }
>    }; 
>   }
> 
Hmm, I wonder if it's trivial enough to filter out throw statements that are inside a try block scope (even without checking the expression and catch block types)?

================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:32
@@ +31,3 @@
+// token including trailing whitespace.
+static SourceRange FindToken(const SourceManager &Sources, LangOptions LangOpts,
+                             SourceLocation StartLoc, SourceLocation EndLoc,
----------------
Prazek wrote:
> function in llvm starts with lower case
I prefer this be gated on D20428; we should be pushing this functionality down where it belongs instead of replicating it in several different clang-tidy checks.

================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:91
@@ +90,3 @@
+
+SourceRange FindNoExceptRange(const ASTContext &Context,
+                              const SourceManager &Sources,
----------------
Prazek wrote:
> rename it to findNoexceptRange at mark it static.
This function will not work with dynamic exception specifications that also specify the function is nonthrowing, e.g., `throw()`.

================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:130
@@ +129,3 @@
+  const auto *Throw = Result.Nodes.getNodeAs<Stmt>("throw");
+  diag(Throw->getLocStart(), "throw in function declared no-throw");
+  for (auto Redecl : Function->redecls()) {
----------------
Prazek wrote:
> add new line after this line
How about wording the diagnostic: "'throw' expression in a function declared with a non-throwing exception specification"

================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.cpp:131
@@ +130,3 @@
+  diag(Throw->getLocStart(), "throw in function declared no-throw");
+  for (auto Redecl : Function->redecls()) {
+    SourceRange NoExceptRange =
----------------
Prazek wrote:
> add some new line
`const auto *`, please (assuming Redecl is a pointer type).

================
Comment at: clang-tidy/misc/ThrowWithNoexceptCheck.h:20
@@ +19,3 @@
+///\brief Warns about using throw in function declared as noexcept.
+/// It complains about every throw, even if it is caught later.
+class ThrowWithNoexceptCheck : public ClangTidyCheck {
----------------
What is the false positive rate with this check over a large codebase that uses exceptions?

================
Comment at: test/clang-tidy/misc-throw-with-noexcept.cpp:70
@@ +69,2 @@
+}
+// CHECK-MESSAGES: :[[@LINE-2]]:2: warning: throw in function declared no-throw [misc-throw-with-noexcept]
----------------
There are no tests for dynamic exception specifications.


Repository:
  rL LLVM

http://reviews.llvm.org/D19201





More information about the cfe-commits mailing list