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