[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp:88
+
+  for (auto [ThrowType, ThrowLoc] : AnalyzeResult.getExceptionTypes())
+    diag(ThrowLoc, "may throw %0 here", DiagnosticIDs::Note)
----------------



================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:14
 void ExceptionAnalyzer::ExceptionInfo::registerException(
-    const Type *ExceptionType) {
+    const Type *ExceptionType, const SourceLocation Loc) {
   assert(ExceptionType != nullptr && "Only valid types are accepted");
----------------
Is there any particular reason for taking `SourceLocation` by value? A `const SourceLocation &` would avoid an unnecessary copy.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:21
 void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
-    const Throwables &Exceptions) {
-  if (Exceptions.size() == 0)
+    const Throwables &Exceptions, const SourceLocation Loc) {
+  if (Exceptions.empty())
----------------
Same question here regarding the argument type.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:25
   Behaviour = State::Throwing;
-  ThrownExceptions.insert(Exceptions.begin(), Exceptions.end());
+  for (const auto [ThrowType, ThrowLoc] : Exceptions)
+    ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
----------------



================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+  for (const auto [ThrowType, ThrowLoc] : Exceptions)
+    ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
 }
----------------
There shouldn't be any invalid location in the container. An exception is thrown by a statement, so we know where in source that happens.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:352
+
+  auto ShouldRemoveFnt = [HandlerTy, &Context](const Type *ExceptionTy) {
     CanQualType ExceptionCanTy = ExceptionTy->getCanonicalTypeUnqualified();
----------------
For a moment it wasn't entirely clear for me what `Fnt` meant, I suggest using naming like `FnType` or `FunctionType`.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:391
+        return true;
+      return false;
     }
----------------



================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:408
 
-  for (const Type *T : TypesToDelete)
-    ThrownExceptions.erase(T);
+    CaughtExceptions.emplace(*It);
+    It = ThrownExceptions.erase(It);
----------------
This introduces a side effect to this function and makes it do 2 things at the same time. Besides filtering the caught exceptions, it also collects and returns them through a reference parameter.

I don't mind the latter, but in that case I'd like to see the caught exceptions returned from the function as a part of the return statement. Also notice that if `CaughtExceptions` is not empty, `Result` is `true`, which makes the latter redundant. I suggest dropping the `bool` return type and returning the `Throwables` instead.




================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+            (IgnoredTypes.count(TD->getName()) > 0)) {
+          It = ThrownExceptions.erase(It);
+          continue;
----------------
You are erasing something from a container on which you're iteration over. Are you sure the iterators are not invalidated, when the internal representation changes?


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:542
+                                   CaughtExceptions)) {
+          CaughtExceptions.emplace(CaughtType, SourceLocation());
           ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),
----------------
Here we rethrow something from a `try` block if I understand it correctly.

```lang=c++
void foo() {
    throw 3;
}

void bar() {
    try {
        foo();
    } catch(short) {
    }
}
```

The best way would be to set the `SourceLocation` to the point, where `foo()` is called.
I think you would need to create a new `struct` called `ThrowableInfo`, which contains, 
every information that we need to know about the `Throwable` e.g.: type, location, parent
context, etc. That would also be more extensible.

If there's no way to deduce this location for some reason, you can still set the location 
to the `try` block and create messages like `rethrown from try here`, etc. Just don't create
invalid `SourceLocation`s please, because besides them being incorrect, they are also
a source of bugs and crashes.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:573
                                   Excs.getExceptionTypes(), CallStack));
-    for (const Type *Throwable : Excs.getExceptionTypes()) {
+    for (auto [Throwable, ThrowLoc] : Excs.getExceptionTypes()) {
       if (const auto ThrowableRec = Throwable->getAsCXXRecordDecl()) {
----------------



================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h:40
   /// exception at runtime.
+
   class ExceptionInfo {
----------------
Remove this `\n` please, we don't have an additional new line above the `ExceptionAnalyzer` class either, so please keep it consistent.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298