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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
