Emit additional notes with information about thrown uncaught
exceptions at a location where they thrown or re-thrown.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
---|---|---|
84–86 | I'm still not sure how I feel about this message though. | |
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp | ||
331 | 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? | |
380–383 | 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.: 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. | |
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h | ||
47 | What is the reason behind declaring this operator and the one below as friend? |
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.
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
---|---|---|
84–86 | 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. | |
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp | ||
331 | No, old behavior were inefficient. All push_backs push same exception into vector, that we later remove from set. So when we execute additional ifs 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. | |
380–383 | 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. As for double throw of same type. I agree, but I don't think its worth being implemented currently. | |
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h | ||
47 | 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. |
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp | ||
---|---|---|
380–383 | I still feel like creating these {T, SourceLocation()} entries is bloated. In case of a map, you wouldn't need to create dummy SourceLocations. We have DenseMap, which is documented like this:
Here you would map pointers to SourceLocations, which are basically ints, 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. |
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp | ||
---|---|---|
380–383 | For SourceLocation{}, I can add constructors to ThrowInfo to hide it, but that's also not elegant solution. |
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp | ||
---|---|---|
80 | ||
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp | ||
14 | Is there any particular reason for taking SourceLocation by value? A const SourceLocation & would avoid an unnecessary copy. | |
21–22 | Same question here regarding the argument type. | |
25–26 | ||
26 | There shouldn't be any invalid location in the container. An exception is thrown by a statement, so we know where in source that happens. | |
324 | For a moment it wasn't entirely clear for me what Fnt meant, I suggest using naming like FnType or FunctionType. | |
363 | ||
380–383 | This introduces a side effect to this function and makes it do 2 things at the same time. Besides filtering the caught exceptions, it also collects and returns them through a reference parameter. I don't mind the latter, but in that case I'd like to see the caught exceptions returned from the function as a part of the return statement. Also notice that if CaughtExceptions is not empty, Result is true, which makes the latter redundant. I suggest dropping the bool return type and returning the Throwables instead. | |
400 | You are erasing something from a container on which you're iteration over. Are you sure the iterators are not invalidated, when the internal representation changes? | |
514 | Here we rethrow something from a try block if I understand it correctly. void foo() { throw 3; } void bar() { try { foo(); } catch(short) { } } The best way would be to set the SourceLocation to the point, where foo() is called. If there's no way to deduce this location for some reason, you can still set the location | |
514 | Also, is this branch tested? I don't see any new test case with a try block. | |
545 | ||
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h | ||
40 | Remove this \n please, we don't have an additional new line above the ExceptionAnalyzer class either, so please keep it consistent. |
TODO: Resolve rest of review comments.
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp | ||
---|---|---|
14 | SourceLocation is 4 bytes in size (encoded as enum). In clang is always passed by value, | |
21–22 | Same answer, 4 bytes in size. | |
26 | yes and no, this is an safety... and we may still not see an "throw" if we call function that explicitly declare that throw some exceptions, but we do not have definition. | |
400 | erase returns new valid iterator |