[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL abandoned this revision. PiotrZSL marked 4 inline comments as done. PiotrZSL added a comment. I do not plan to work on this check anymore, sorry for a wasted time. 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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL planned changes to this revision.
PiotrZSL marked 4 inline comments as done.
PiotrZSL added a comment.
TODO: Resolve rest of review comments.
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");
isuckatcs wrote:
> Is there any particular reason for taking `SourceLocation` by value? A `const
> SourceLocation &` would avoid an unnecessary copy.
SourceLocation is 4 bytes in size (encoded as enum). In clang is always passed
by value,
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:21
void ExceptionAnalyzer::ExceptionInfo::registerExceptions(
-const Throwables ) {
- if (Exceptions.size() == 0)
+const Throwables , const SourceLocation Loc) {
+ if (Exceptions.empty())
isuckatcs wrote:
> Same question here regarding the argument type.
Same answer, 4 bytes in size.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:26
+ for (const auto [ThrowType, ThrowLoc] : Exceptions)
+ThrownExceptions.emplace(ThrowType, ThrowLoc.isInvalid() ? Loc : ThrowLoc);
}
isuckatcs wrote:
> 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.
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.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:428
+(IgnoredTypes.count(TD->getName()) > 0)) {
+ It = ThrownExceptions.erase(It);
+ continue;
isuckatcs wrote:
> 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?
`erase` returns new valid iterator
https://en.cppreference.com/w/cpp/container/map/erase
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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
isuckatcs added inline comments.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:542
+ CaughtExceptions)) {
+ CaughtExceptions.emplace(CaughtType, SourceLocation());
ExceptionInfo Rethrown = throwsException(Catch->getHandlerBlock(),
isuckatcs wrote:
> 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.
Also, is this branch tested? I don't see any new test case with a `try` block.
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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
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 ) {
- if (Exceptions.size() == 0)
+const Throwables , 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, ](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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL updated this revision to Diff 551761.
PiotrZSL added a comment.
Rebase + Ping
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153298/new/
https://reviews.llvm.org/D153298
Files:
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-rethrow.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -9,6 +9,7 @@
struct throwing_destructor {
~throwing_destructor() {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~throwing_destructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -16,6 +17,7 @@
struct throwing_move_constructor {
throwing_move_constructor(throwing_move_constructor&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'throwing_move_constructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -23,12 +25,14 @@
struct throwing_move_assignment {
throwing_move_assignment& operator=(throwing_move_assignment&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
void throwing_noexcept() noexcept {
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: may throw 'int' here
throw 1;
}
@@ -42,6 +46,7 @@
void throw_and_catch_some(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch_some' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -70,6 +75,7 @@
void throw_and_rethrow() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_rethrow' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+2]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -79,6 +85,7 @@
void throw_catch_throw() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_throw' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -88,6 +95,7 @@
void throw_catch_rethrow_the_rest(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_rethrow_the_rest' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -120,6 +128,7 @@
void throw_catch_multi_ptr_1() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_multi_ptr_1' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'char **' here
try {
char **p = 0;
throw p;
@@ -165,6 +174,7 @@
void throw_c_catch_pointer() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try {
int a = 1;
const int *p =
@@ -174,6 +184,7 @@
void throw_c_catch_pointer_v() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer_v' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try {
int a = 1;
const int *p =
@@ -183,6 +194,7 @@
void throw_v_catch_pointer() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_v_catch_pointer' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'volatile int *' here
try {
int a = 1;
volatile int *p =
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL added a comment. Ping 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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL updated this revision to Diff 534169.
PiotrZSL added a comment.
Use std::map
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153298/new/
https://reviews.llvm.org/D153298
Files:
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-rethrow.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -9,6 +9,7 @@
struct throwing_destructor {
~throwing_destructor() {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~throwing_destructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -16,6 +17,7 @@
struct throwing_move_constructor {
throwing_move_constructor(throwing_move_constructor&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'throwing_move_constructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -23,12 +25,14 @@
struct throwing_move_assignment {
throwing_move_assignment& operator=(throwing_move_assignment&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
void throwing_noexcept() noexcept {
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: may throw 'int' here
throw 1;
}
@@ -42,6 +46,7 @@
void throw_and_catch_some(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch_some' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -70,6 +75,7 @@
void throw_and_rethrow() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_rethrow' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+2]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -79,6 +85,7 @@
void throw_catch_throw() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_throw' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -88,6 +95,7 @@
void throw_catch_rethrow_the_rest(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_rethrow_the_rest' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -120,6 +128,7 @@
void throw_catch_multi_ptr_1() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_multi_ptr_1' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'char **' here
try {
char **p = 0;
throw p;
@@ -152,7 +161,7 @@
}
// FIXME: In this case 'a' is convertible to the handler and should be caught
-// but in reality it's thrown. Note that clang doesn't report a warning for
+// but in reality it's thrown. Note that clang doesn't report a warning for
// this either.
void throw_catch_multi_ptr_5() noexcept {
try {
@@ -165,6 +174,7 @@
void throw_c_catch_pointer() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try {
int a = 1;
const int *p =
@@ -174,6 +184,7 @@
void throw_c_catch_pointer_v() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer_v' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try {
int a = 1;
const int *p =
@@
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
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` 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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
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` 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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
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` 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 , const ThrowInfo ) 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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL updated this revision to Diff 532783.
PiotrZSL marked 3 inline comments as done.
PiotrZSL added a comment.
Review fixes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153298/new/
https://reviews.llvm.org/D153298
Files:
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-rethrow.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -9,6 +9,7 @@
struct throwing_destructor {
~throwing_destructor() {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~throwing_destructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -16,6 +17,7 @@
struct throwing_move_constructor {
throwing_move_constructor(throwing_move_constructor&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'throwing_move_constructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -23,12 +25,14 @@
struct throwing_move_assignment {
throwing_move_assignment& operator=(throwing_move_assignment&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
void throwing_noexcept() noexcept {
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: may throw 'int' here
throw 1;
}
@@ -42,6 +46,7 @@
void throw_and_catch_some(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch_some' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -70,6 +75,7 @@
void throw_and_rethrow() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_rethrow' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+2]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -79,6 +85,7 @@
void throw_catch_throw() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_throw' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -88,6 +95,7 @@
void throw_catch_rethrow_the_rest(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_rethrow_the_rest' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -120,6 +128,7 @@
void throw_catch_multi_ptr_1() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_multi_ptr_1' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'char **' here
try {
char **p = 0;
throw p;
@@ -152,7 +161,7 @@
}
// FIXME: In this case 'a' is convertible to the handler and should be caught
-// but in reality it's thrown. Note that clang doesn't report a warning for
+// but in reality it's thrown. Note that clang doesn't report a warning for
// this either.
void throw_catch_multi_ptr_5() noexcept {
try {
@@ -165,6 +174,7 @@
void throw_c_catch_pointer() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try {
int a = 1;
const int *p =
@@ -174,6 +184,7 @@
void throw_c_catch_pointer_v() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer_v' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
isuckatcs added inline comments.
Comment at: clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp:84-86
+ if (AnalyzeResult.containsUnknownElements())
+diag(MatchedDecl->getLocation(), "may throw unknown exceptions",
+ DiagnosticIDs::Note);
I'm still not sure how I feel about this message though.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:334
+ CaughtExceptions.insert({ExceptionTy, ThrowLoc});
+ continue;
}
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?
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:390
for (const Type *T : TypesToDelete)
-ThrownExceptions.erase(T);
+ThrownExceptions.erase({T, SourceLocation()});
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` would be better.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h:46
+
+ friend bool operator<(const ThrowInfo , const ThrowInfo ) noexcept {
+return L.ThrowType < R.ThrowType;
What is the reason behind declaring this operator and the one below as `friend`?
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
[PATCH] D153298: [clang-tidy] Extend bugprone-exception-escape diagnostics
PiotrZSL created this revision.
PiotrZSL added reviewers: njames93, carlosgalvezp, isuckatcs, JonasToth,
baloghadamsoftware.
Herald added a subscriber: xazax.hun.
Herald added a project: All.
PiotrZSL requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Emit additional notes with information about thrown uncaught
exceptions at a location where they thrown or re-thrown.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D153298
Files:
clang-tools-extra/clang-tidy/bugprone/ExceptionEscapeCheck.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-rethrow.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp
@@ -9,6 +9,7 @@
struct throwing_destructor {
~throwing_destructor() {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function '~throwing_destructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -16,6 +17,7 @@
struct throwing_move_constructor {
throwing_move_constructor(throwing_move_constructor&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'throwing_move_constructor' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
@@ -23,12 +25,14 @@
struct throwing_move_assignment {
throwing_move_assignment& operator=(throwing_move_assignment&&) {
// CHECK-MESSAGES: :[[@LINE-1]]:29: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
+// CHECK-MESSAGES: :[[@LINE+1]]:5: note: may throw 'int' here
throw 1;
}
};
void throwing_noexcept() noexcept {
-// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throwing_noexcept' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+1]]:3: note: may throw 'int' here
throw 1;
}
@@ -42,6 +46,7 @@
void throw_and_catch_some(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_catch_some' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -70,6 +75,7 @@
void throw_and_rethrow() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_and_rethrow' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+2]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -79,6 +85,7 @@
void throw_catch_throw() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_throw' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'int' here
try {
throw 1;
} catch(int &) {
@@ -88,6 +95,7 @@
void throw_catch_rethrow_the_rest(int n) noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_rethrow_the_rest' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'double' here
try {
if (n) throw 1;
throw 1.1;
@@ -120,6 +128,7 @@
void throw_catch_multi_ptr_1() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_multi_ptr_1' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+3]]:5: note: may throw 'char **' here
try {
char **p = 0;
throw p;
@@ -152,7 +161,7 @@
}
// FIXME: In this case 'a' is convertible to the handler and should be caught
-// but in reality it's thrown. Note that clang doesn't report a warning for
+// but in reality it's thrown. Note that clang doesn't report a warning for
// this either.
void throw_catch_multi_ptr_5() noexcept {
try {
@@ -165,6 +174,7 @@
void throw_c_catch_pointer() noexcept {
// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_c_catch_pointer' which should not throw exceptions
+ // CHECK-MESSAGES: :[[@LINE+4]]:5: note: may throw 'const int *' here
try {
int a = 1;
const int *p =
@@ -174,6 +184,7 @@
void
