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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL planned changes to this revision.
PiotrZSL marked 4 inline comments as done.
PiotrZSL added a comment.

TODO: Resolve rest of review comments.



================
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");
----------------
isuckatcs wrote:
> Is there any particular reason for taking `SourceLocation` by value? A `const SourceLocation &` would avoid an unnecessary copy.
SourceLocation is 4 bytes in size (encoded as enum). In clang is always passed by value,


================
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())
----------------
isuckatcs wrote:
> Same question here regarding the argument type.
Same answer, 4 bytes in size.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+  for (const auto [ThrowType, ThrowLoc] : Exceptions)
+    ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
 }
----------------
isuckatcs wrote:
> 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.
yes and no, this is an safety... and we may still not see an "throw" if we call function that explicitly declare that throw some exceptions, but we do not have definition.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+            (IgnoredTypes.count(TD->getName()) > 0)) {
+          It = ThrownExceptions.erase(It);
+          continue;
----------------
isuckatcs wrote:
> 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?
`erase` returns new valid iterator
https://en.cppreference.com/w/cpp/container/map/erase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298