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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:100
+
+namespace {
+
----------------
> make anonymous namespaces as small as possible, and only use them for class declarations

http://llvm.org/docs/CodingStandards.html#anonymous-namespaces


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:105
+const TypeVec throwsException(const FunctionDecl *Func) {
+  static thread_local llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+
----------------
I don't think this is a good use case for a static local variable. If this function needs a state, either pass it from the caller or create a utility class/struct for it. You can leave a non-recursive entry point with the current interface, if you like. For example:

  const TypeVec throwsException(const FunctionDecl *Func, llvm::SmallSet<const FunctionDecl *, 32> *CallStack) { ... }

  const TypeVec throwsException(const FunctionDecl *Func) {
    llvm::SmallSet<const FunctionDecl *, 32> CallStack;
    return throwsException(Func, &CallStack);
  }
  


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:110
+
+  if (const auto *Body = Func->getBody()) {
+    CallStack.insert(Func);
----------------
Please use the concrete type here, since it's not obvious from the context.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:119
+  if (const auto *FPT = Func->getType()->getAs<FunctionProtoType>()) {
+    for (const auto Ex : FPT->exceptions()) {
+      Result.push_back(&*Ex);
----------------
Please use the concrete type here.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:120
+    for (const auto Ex : FPT->exceptions()) {
+      Result.push_back(&*Ex);
+    }
----------------
Ex.getTypePtrOrNull() / Ex.getTypePtr() would be easier to understand here.


================
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) {
----------------
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.


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:166
+        }
+        auto *NewEnd = std::remove_if(Uncaught.begin(), Uncaught.end(),
+                                      [&CaughtType](const Type *ThrownType) {
----------------
1. iterator being a pointer is an implementation detail of SmallVector
2. use llvm::remove_if, it has a range based interface


================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:176
+          Results.append(Rethrown.begin(), Rethrown.end());
+          Uncaught.erase(NewEnd, Uncaught.end());
+        }
----------------
I'd put this as close to the remove_if as possible to allow readers keep less context to understand the code. In this case - as the first statement inside the `if`.


https://reviews.llvm.org/D33537