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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL 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()});
 
----------------
isuckatcs wrote:
> 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. 
For SourceLocation{}, I can add constructors to ThrowInfo to hide it, but that's also not elegant solution.
Thing is that llvm::SmallSet here wont allocate memory for up to 2 elements, this mean no memory allocation and so on.
And because on every function we basically doing a copy of this container, or we create new one, then allocating memory with DenseMap could be potentially costly (allocate 64 elements).
I also run into compilation issues when using const Type* as a key for a DenseMap (yes I tried it before I choose this solution).
If we talk about std::map, well, that is a better option, as it's not allocate memory by default.
I can give it a try...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153298