[PATCH] D33537: [clang-tidy] Exception Escape Checker

[Alexander Kornienko via Phabricator via cfe-commits]

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

Thank you for the updates. A few more comments.



================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:150
+  } else if (const auto *Try = dyn_cast<CXXTryStmt>(St)) {
+    auto Uncaught = throwsException(Try->getTryBlock(), Caught);
+    for (unsigned i = 0; i < Try->getNumHandlers(); ++i) {
----------------
alexfh wrote:
> Please use the concrete type here and below. http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable says 
> 
> > Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context.
I don't remember where this comment was, but now I see a few more instances of the same issue. Specifically, ThrownExpr and ThrownType here and CaughtType below.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:17
+
+using namespace clang;
+using namespace clang::ast_matchers;
----------------
You can open namespace clang here instead of this using directive.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:22
+typedef llvm::SmallVector<const Type *, 8> TypeVec;
+typedef llvm::SmallSet<StringRef, 8> StringSet;
+} // namespace
----------------
There's `llvm::StringSet<>` in llvm/ADT/StringSet.h.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:25
+
+static const TypeVec throwsException(const FunctionDecl *Func);
+
----------------
Too many forward declarations for my taste. Can you just move all static functions here and remove unnecessary forward declarations? I guess, one should be enough for all the functions here.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:178
+        }
+        auto *NewEnd =
+            llvm::remove_if(Uncaught, [&CaughtType](const Type *ThrownType) {
----------------
iterator being a pointer is an implementation detail of SmallVector. s/auto \*/auto /


================
Comment at: docs/ReleaseNotes.rst:230
 
+<<<<<<< HEAD
+=======
----------------
Resolve the conflicts.


================
Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:28
+
+   Comma separated list containing function names which should not throw. An
+   example for using this parameter is the function ``WinMain()`` in the
----------------
baloghadamsoftware wrote:
> dberris wrote:
> > `EnabledFunctions` but they *should not* throw?
> Maybe we should come up with a better name for this option. I just took this from our company internal check.
FunctionsThatShouldNotThrow?


================
Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:29
+   Comma separated list containing function names which should not throw. An
+   example for using this parameter is the function ``WinMain()`` in the
+   Windows API. Default vale is empty string.
----------------
Examples should be copy-pastable. I suppose, one can't use `WinMain()` verbatim as the value of this option. An example could be: `main,WinMain` or something like that.


================
Comment at: docs/clang-tidy/checks/bugprone-exception-escape.rst:30
+   example for using this parameter is the function ``WinMain()`` in the
+   Windows API. Default vale is empty string.
+
----------------
s/vale/value/


https://reviews.llvm.org/D33537