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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
