isuckatcs added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp:88
+
+ for (auto [ThrowType, ThrowLoc] : AnalyzeResult.getExceptionTypes())
+ diag(ThrowLoc, "may throw %0 here", DiagnosticIDs::Note)
----------------
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:14
void ExceptionAnalyzer::ExceptionInfo::registerException(
- const Type *ExceptionType) {
+ const Type *ExceptionType, const SourceLocation Loc) {
assert(ExceptionType != nullptr && "Only valid types are accepted");
----------------
Is there any particular reason for taking `SourceLocation` by value? A `const
SourceLocation &` would avoid an unnecessary copy.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:21
void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
- const Throwables &Exceptions) {
- if (Exceptions.size() == 0)
+ const Throwables &Exceptions, const SourceLocation Loc) {
+ if (Exceptions.empty())
----------------
Same question here regarding the argument type.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:25
Behaviour = State::Throwing;
- ThrownExceptions.insert(Exceptions.begin(), Exceptions.end());
+ for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
----------------
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+ for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
}
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:352
+
+ auto ShouldRemoveFnt = [HandlerTy, &Context](const Type *ExceptionTy) {
CanQualType ExceptionCanTy = ExceptionTy->getCanonicalTypeUnqualified();
----------------
For a moment it wasn't entirely clear for me what `Fnt` meant, I suggest using
naming like `FnType` or `FunctionType`.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:391
+ return true;
+ return false;
}
----------------
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:408
- for (const Type *T : TypesToDelete)
- ThrownExceptions.erase(T);
+ CaughtExceptions.emplace(*It);
+ It = ThrownExceptions.erase(It);
----------------
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.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+ (IgnoredTypes.count(TD->getName()) > 0)) {
+ It = ThrownExceptions.erase(It);
+ continue;
----------------
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?
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:542
+ CaughtExceptions)) {
+ CaughtExceptions.emplace(CaughtType, SourceLocation());
ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),
----------------
Here we rethrow something from a `try` block if I understand it correctly.
```lang=c++
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.
I think you would need to create a new `struct` called `ThrowableInfo`, which
contains,
every information that we need to know about the `Throwable` e.g.: type,
location, parent
context, etc. That would also be more extensible.
If there's no way to deduce this location for some reason, you can still set
the location
to the `try` block and create messages like `rethrown from try here`, etc. Just
don't create
invalid `SourceLocation`s please, because besides them being incorrect, they
are also
a source of bugs and crashes.
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:573
Excs.getExceptionTypes(), CallStack));
- for (const Type *Throwable : Excs.getExceptionTypes()) {
+ for (auto [Throwable, ThrowLoc] : Excs.getExceptionTypes()) {
if (const auto ThrowableRec = Throwable->getAsCXXRecordDecl()) {
----------------
================
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h:40
/// exception at runtime.
+
class ExceptionInfo {
----------------
Remove this `\n` please, we don't have an additional new line above the
`ExceptionAnalyzer` class either, so please keep it consistent.
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
