[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:84-86
+  if (AnalyzeResult.containsUnknownElements())
+    diag(MatchedDecl->getLocation(), "may throw unknown exceptions",
+         DiagnosticIDs::Note);
----------------
I'm still not sure how I feel about this message though.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:334
+      CaughtExceptions.insert({ExceptionTy, ThrowLoc});
+      continue;
     }
----------------
This `continue` and the other one change the behaviour of this function.  Without this some additional conditions are also checked after the `else if` block. Shouldn't we preserve the old behaviour?


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:390
   for (const Type *T : TypesToDelete)
-    ThrownExceptions.erase(T);
+    ThrownExceptions.erase({T, SourceLocation()});
 
----------------
This line makes me wonder if it's worth using a `map` instead of a `set` for `ThrownExceptions`. You could map the type to the location. I mean technically, that happens now too, but not with the appropriate data structure. 

Also I wonder what happens if a function can throw the same type from multiple locations. E.g.:
```lang=c++
void foo(int x) {
  if(x == 0) 
    throw 1;
  
  if(x == 1)
    throw 2;
}
```
Here only the last location will be preserved, so maybe mapping `Type` to `vector<SourceLocation>` would be better.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h:46
+
+      friend bool operator<(const ThrowInfo &L, const ThrowInfo &R) noexcept {
+        return L.ThrowType < R.ThrowType;
----------------
What is the reason behind declaring this operator and the one below as `friend`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298