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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Question would be, does this can stay +- like this and we could wait with extension this until some users for example complain, that they would like it to be extended, or this need to be done now.
Main reason for this change is, that often I run into situation when there were some warning raised for some function, but after deeper investigation it were for example due to some exceptions being thrown in some boost library.
And that hard to guess were the issue were.



================
Comment at: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp:84-86
+  if (AnalyzeResult.containsUnknownElements())
+    diag(MatchedDecl->getLocation(), "may throw unknown exceptions",
+         DiagnosticIDs::Note);
----------------
isuckatcs wrote:
> I'm still not sure how I feel about this message though.
I also, my main reason was to show user that except listed exceptions, also some other exceptions may be thrown, that we do not know because they throw from an functions without implementation.
In ideal condition I should just raise this warning for a function that we called, that we do not have implementation, and we do not know what it throws.
I would say that this could be improved in future. Originally I raised all those notes against a main function, changed this recently to show were throws are called.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:334
+      CaughtExceptions.insert({ExceptionTy, ThrowLoc});
+      continue;
     }
----------------
isuckatcs wrote:
> 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?
No, old behavior were inefficient. All push_backs push same exception into vector, that we later remove from set. So when we execute additional `if`s we still going to push same execution to vector, and as a result we going to try to remove same element from set twice. That`s not needed.


================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:390
   for (const Type *T : TypesToDelete)
-    ThrownExceptions.erase(T);
+    ThrownExceptions.erase({T, SourceLocation()});
 
----------------
isuckatcs wrote:
> 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.
We use llvm::SmallSet, but there is no llvm::SmallMap. I wanted to preserve memory optimizations, as using some map could hit performance, after all we create many of those temporary containers.
And with map I run into some issues (it didn't like const pointer as key).

As for double throw of same type. I agree, but I don't think its worth being implemented currently.
Once user remove one exception, check will show second. Many checks work like that.
This is just to provide small hint.


================
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;
----------------
isuckatcs wrote:
> What is the reason behind declaring this operator and the one below as `friend`?
I just get used to do that. Usually friend enable conversion, but here we do not have constructor or derived classes, so that's not needed.
I can change this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298