PiotrZSL added a comment.
Question would be, does this can stay +- like this and we could wait with
extension this until some users for example complain, that they would like it
to be extended, or this need to be done now.
Main reason for this change is, that often I run into situation when there were
some warning raised for some function, but after deeper investigation it were
for example due to some exceptions being thrown in some boost library.
And that hard to guess were the issue were.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp:84-86
+ if (AnalyzeResult.containsUnknownElements())
+ diag(MatchedDecl->getLocation(), "may throw unknown exceptions",
+ DiagnosticIDs::Note);
----------------
isuckatcs wrote:
> I'm still not sure how I feel about this message though.
I also, my main reason was to show user that except listed exceptions, also
some other exceptions may be thrown, that we do not know because they throw
from an functions without implementation.
In ideal condition I should just raise this warning for a function that we
called, that we do not have implementation, and we do not know what it throws.
I would say that this could be improved in future. Originally I raised all
those notes against a main function, changed this recently to show were throws
are called.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:334
+ CaughtExceptions.insert({ExceptionTy, ThrowLoc});
+ continue;
}
----------------
isuckatcs wrote:
> This `continue` and the other one change the behaviour of this function.
> Without this some additional conditions are also checked after the `else if`
> block. Shouldn't we preserve the old behaviour?
No, old behavior were inefficient. All push_backs push same exception into
vector, that we later remove from set. So when we execute additional `if`s we
still going to push same execution to vector, and as a result we going to try
to remove same element from set twice. That`s not needed.
================
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:
> 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.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h:46
+
+ friend bool operator<(const ThrowInfo &L, const ThrowInfo &R) noexcept {
+ return L.ThrowType < R.ThrowType;
----------------
isuckatcs wrote:
> What is the reason behind declaring this operator and the one below as
> `friend`?
I just get used to do that. Usually friend enable conversion, but here we do
not have constructor or derived classes, so that's not needed.
I can change this.
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
