[PATCH] D153458: [clang-tidy] Model noexcept more properly in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:319
 
+static bool cannotThrow(const FunctionDecl *Func) {
+  const auto *FunProto = Func->getType()->getAs<FunctionProtoType>();
----------------
isuckatcs wrote:
> Put this in the anonymous namespace above please to remain consistent.
well, llvm style require `static`, if we want to be consistent, I can change all other into static later.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:446
+  if (!Func || CallStack.count(Func) ||
+      (!CallStack.empty() && cannotThrow(Func)))
     return ExceptionInfo::createNonThrowing();
----------------
isuckatcs wrote:
> Is `cannotThrow(Func)` really needed here? Isn't it possible to bail out after the body of the function has been analyzed? 
> 
> I understand that you want to prevent some recursive calls and bail out early, but I don't think that it worths adding some additional logic, which is not needed anyway. 
> 
> If you really want to optimize this or you're worried about stack overflows, consider rewriting the recursive solution to an iterative one.
Yes, it should be here because it's called in 3 places so that an best place to put it.
It's not about recursion or performance, it's already exist and I simply do no change it.

Simply if we analyse callExpr to function/destructor/constructor/... and that FunctionDecl is noexcept, then we should jump to that function and analyse statements in that function body at all.
And this is what is done here. If this isn't first function `!CallStack.empty()` (aka: call from check code), and it's `noexcept`, then assume that it cannot throw and do not go deeper.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153458/new/

https://reviews.llvm.org/D153458