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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to