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