[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/utils/ExceptionAnalyzer.cpp:390
   for (const Type *T : TypesToDelete)
-    ThrownExceptions.erase(T);
+    ThrownExceptions.erase({T, SourceLocation()});
 
----------------
PiotrZSL wrote:
> 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.
I still feel like creating these `{T, SourceLocation()}` entries is bloated. In case of a map, you wouldn't need to create dummy `SourceLocation`s. 

We have `DenseMap`, which is documented like this:
> DenseMap is a simple quadratically probed hash table. It excels at supporting small keys and values: [...] DenseMap is a great way to map pointers to pointers, or map other small types to each other.

Here you would map pointers to `SourceLocation`s, which are basically `int`s, so they are smaller than pointers. I think it worths giving `DenseMap` a try.

Also note that the number of exceptions we store is very low, so even `std::map<>` wouldn't cause a significant performance loss. Also currently our `SmallSet<>` is set to 2 elements, which means if we store more than 2 elements, it will switch to `std::set<>` instead. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298