[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.

It looks like you've missed some comments or uploaded a wrong patch.



================
Comment at: clang-tidy/bugprone/ExceptionEscapeCheck.cpp:105
+const TypeVec throwsException(const FunctionDecl *Func) {
+  static thread_local llvm::SmallSet<const FunctionDecl *, 32> CallStack;
+
----------------
alexfh wrote:
> 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);
>   }
>   
Still not addressed?


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


================
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);
----------------
alexfh wrote:
> Please use the concrete type here.
Still not addressed?


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


https://reviews.llvm.org/D33537