[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
xazax.hun accepted this revision. xazax.hun added a comment. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
isuckatcs added inline comments.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:66
+ CXXBasePath &) {
+if (BS->getType()->getAsCXXRecordDecl() == BaseClass &&
+BS->getAccessSpecifier() == AS_public) {
xazax.hun wrote:
> Will this work with typedefs and other sugar or do you need to get the
> canonical type here? I do not see test coverage for that scenario.
In this specific case it will work because `getAsCXXRecordDecl()` comes from
the `CanProxyBase` utility class, which already canonicalizes the type, but I
agree that it's not intuitive and unreadable.
I also modified the current test cases to have aliases.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135495/new/
https://reviews.llvm.org/D135495
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
isuckatcs updated this revision to Diff 499234.
isuckatcs marked an inline comment as done.
isuckatcs added a comment.
Addressed comments
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135495/new/
https://reviews.llvm.org/D135495
Files:
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
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
@@ -101,6 +101,126 @@
}
}
+void throw_catch_pointer_c() noexcept {
+ try {
+int a = 1;
+throw
+ } catch(const int *) {}
+}
+
+void throw_catch_pointer_v() noexcept {
+ try {
+int a = 1;
+throw
+ } catch(volatile int *) {}
+}
+
+void throw_catch_pointer_cv() noexcept {
+ try {
+int a = 1;
+throw
+ } catch(const volatile int *) {}
+}
+
+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
+ try {
+char **p = 0;
+throw p;
+ } catch (const char **) {
+ }
+}
+
+void throw_catch_multi_ptr_2() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (const char *const *) {
+ }
+}
+
+void throw_catch_multi_ptr_3() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (volatile char *const *) {
+ }
+}
+
+void throw_catch_multi_ptr_4() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (volatile const char *const *) {
+ }
+}
+
+// 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
+// this either.
+void throw_catch_multi_ptr_5() noexcept {
+ try {
+double *a[2][3];
+throw a;
+ } catch (double *(*)[3]) {
+ }
+}
+
+
+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
+ try {
+int a = 1;
+const int *p =
+throw p;
+ } catch(int *) {}
+}
+
+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
+ try {
+int a = 1;
+const int *p =
+throw p;
+ } catch(volatile int *) {}
+}
+
+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
+ try {
+int a = 1;
+volatile int *p =
+throw p;
+ } catch(int *) {}
+}
+
+void throw_v_catch_pointer_c() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_v_catch_pointer_c' which should not throw exceptions
+ try {
+int a = 1;
+volatile int *p =
+throw p;
+ } catch(const int *) {}
+}
+
+void throw_cv_catch_pointer_c() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_cv_catch_pointer_c' which should not throw exceptions
+ try {
+int a = 1;
+const volatile int *p =
+throw p;
+ } catch(const int *) {}
+}
+
+void throw_cv_catch_pointer_v() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_cv_catch_pointer_v' which should not throw exceptions
+ try {
+int a = 1;
+const volatile int *p =
+throw p;
+ } catch(volatile int *) {}
+}
+
class base {};
class derived: public base {};
@@ -112,6 +232,173 @@
}
}
+void throw_derived_alias_catch_base() noexcept {
+ using alias = derived;
+
+ try {
+throw alias();
+ } catch(base &) {
+ }
+}
+
+void throw_derived_catch_base_alias() noexcept {
+ using alias = base;
+
+ try {
+throw derived();
+ } catch(alias &) {
+ }
+}
+
+void throw_derived_catch_base_ptr_c() noexcept {
+ try {
+derived d;
+throw
+ } catch(const base *) {
+ }
+}
+
+void throw_derived_catch_base_ptr() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ptr' which should not throw exceptions
+ try {
+derived d;
+const derived *p =
+throw p;
+ } catch(base *) {
+ }
+}
+
+class A {};
+class B : A {};
+
+// The following alias hell is deliberately created for testing.
+using aliasedA = A;
+class C : protected aliasedA {};
+
+typedef aliasedA moreAliasedA;
+class D : public moreAliasedA {};
+
+using moreMoreAliasedA = moreAliasedA;
+using aliasedD = D;
+class E : public moreMoreAliasedA, public aliasedD {};
+
+void throw_derived_catch_base_private() noexcept {
+ // CHECK-MESSAGES:
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
isuckatcs marked an inline comment as done.
isuckatcs added inline comments.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:84
+
+QualType getPointeeOrArrayElementQualType(QualType T) {
+ if (T->isAnyPointerType())
xazax.hun wrote:
> What about `Type::getPointeeOrArrayElementType`?
I didn't use that for two reasons.
# It returns a `Type` instead of a `QualType`, so the qualifiers are lost.
# It uses `Type::getBaseElementTypeUnsafe()` for arrays, which discards all
array types for multi-dimensional arrays. E.g.: `int[2][3]` will become `int`
and I want to take only one step backward, so I want `int[2][3]` to become
`int[2]`.
I was thinking about inserting this function into `Type` or `QualType` but
because of the different behaviour with arrays I decided to use it as a helper
in this source file only instead. I agree that the naming is confusing a bit,
but it is literally what the function does.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135495/new/
https://reviews.llvm.org/D135495
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
xazax.hun added a comment.
Overall, I like the structure of this patch and the references back to the
standard. But every time we compare type pointers, they might compare inequal
when one of the types have sugar on it while the other does not. Please review
all of those comparisons to see where do we need to get the canonical types
instead, and add some tests with type aliases.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:66
+ CXXBasePath &) {
+if (BS->getType()->getAsCXXRecordDecl() == BaseClass &&
+BS->getAccessSpecifier() == AS_public) {
Will this work with typedefs and other sugar or do you need to get the
canonical type here? I do not see test coverage for that scenario.
Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:84
+
+QualType getPointeeOrArrayElementQualType(QualType T) {
+ if (T->isAnyPointerType())
What about `Type::getPointeeOrArrayElementType`?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135495/new/
https://reviews.llvm.org/D135495
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
isuckatcs added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135495/new/ https://reviews.llvm.org/D135495 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
isuckatcs updated this revision to Diff 492215.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135495/new/
https://reviews.llvm.org/D135495
Files:
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
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
@@ -101,6 +101,126 @@
}
}
+void throw_catch_pointer_c() noexcept {
+ try {
+int a = 1;
+throw
+ } catch(const int *) {}
+}
+
+void throw_catch_pointer_v() noexcept {
+ try {
+int a = 1;
+throw
+ } catch(volatile int *) {}
+}
+
+void throw_catch_pointer_cv() noexcept {
+ try {
+int a = 1;
+throw
+ } catch(const volatile int *) {}
+}
+
+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
+ try {
+char **p = 0;
+throw p;
+ } catch (const char **) {
+ }
+}
+
+void throw_catch_multi_ptr_2() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (const char *const *) {
+ }
+}
+
+void throw_catch_multi_ptr_3() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (volatile char *const *) {
+ }
+}
+
+void throw_catch_multi_ptr_4() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (volatile const char *const *) {
+ }
+}
+
+// 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
+// this either.
+void throw_catch_multi_ptr_5() noexcept {
+ try {
+double *a[2][3];
+throw a;
+ } catch (double *(*)[3]) {
+ }
+}
+
+
+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
+ try {
+int a = 1;
+const int *p =
+throw p;
+ } catch(int *) {}
+}
+
+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
+ try {
+int a = 1;
+const int *p =
+throw p;
+ } catch(volatile int *) {}
+}
+
+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
+ try {
+int a = 1;
+volatile int *p =
+throw p;
+ } catch(int *) {}
+}
+
+void throw_v_catch_pointer_c() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_v_catch_pointer_c' which should not throw exceptions
+ try {
+int a = 1;
+volatile int *p =
+throw p;
+ } catch(const int *) {}
+}
+
+void throw_cv_catch_pointer_c() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_cv_catch_pointer_c' which should not throw exceptions
+ try {
+int a = 1;
+const volatile int *p =
+throw p;
+ } catch(const int *) {}
+}
+
+void throw_cv_catch_pointer_v() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_cv_catch_pointer_v' which should not throw exceptions
+ try {
+int a = 1;
+const volatile int *p =
+throw p;
+ } catch(volatile int *) {}
+}
+
class base {};
class derived: public base {};
@@ -112,6 +232,84 @@
}
}
+void throw_derived_catch_base_ptr_c() noexcept {
+ try {
+derived d;
+throw
+ } catch(const base *) {
+ }
+}
+
+void throw_derived_catch_base_ptr() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ptr' which should not throw exceptions
+ try {
+derived d;
+const derived *p =
+throw p;
+ } catch(base *) {
+ }
+}
+
+class A {};
+class B : A {};
+class C : protected A {};
+class D : public A {};
+class E : public A, public D {};
+
+void throw_derived_catch_base_private() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private' which should not throw exceptions
+ try {
+B b;
+throw b;
+ } catch(A) {
+ }
+}
+
+void throw_derived_catch_base_private_ptr() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private_ptr' which should not throw exceptions
+ try {
+B b;
+throw
+ } catch(A *) {
+ }
+}
+
+void throw_derived_catch_base_protected() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in
[PATCH] D135495: [clang-tidy] handle exceptions properly `ExceptionAnalyzer`
isuckatcs updated this revision to Diff 492214.
isuckatcs added a comment.
applied clang-format
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D135495/new/
https://reviews.llvm.org/D135495
Files:
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
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
@@ -102,7 +102,6 @@
}
void throw_catch_pointer_c() noexcept {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_pointer_c' which should not throw exceptions
try {
int a = 1;
throw
@@ -110,7 +109,6 @@
}
void throw_catch_pointer_v() noexcept {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_pointer_v' which should not throw exceptions
try {
int a = 1;
throw
@@ -118,13 +116,57 @@
}
void throw_catch_pointer_cv() noexcept {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_catch_pointer_cv' which should not throw exceptions
try {
int a = 1;
throw
} catch(const volatile int *) {}
}
+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
+ try {
+char **p = 0;
+throw p;
+ } catch (const char **) {
+ }
+}
+
+void throw_catch_multi_ptr_2() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (const char *const *) {
+ }
+}
+
+void throw_catch_multi_ptr_3() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (volatile char *const *) {
+ }
+}
+
+void throw_catch_multi_ptr_4() noexcept {
+ try {
+char **p = 0;
+throw p;
+ } catch (volatile const char *const *) {
+ }
+}
+
+// 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
+// this either.
+void throw_catch_multi_ptr_5() noexcept {
+ try {
+double *a[2][3];
+throw a;
+ } catch (double *(*)[3]) {
+ }
+}
+
+
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
try {
@@ -191,7 +233,6 @@
}
void throw_derived_catch_base_ptr_c() noexcept {
- // CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ptr_c' which should not throw exceptions
try {
derived d;
throw
@@ -209,6 +250,66 @@
}
}
+class A {};
+class B : A {};
+class C : protected A {};
+class D : public A {};
+class E : public A, public D {};
+
+void throw_derived_catch_base_private() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private' which should not throw exceptions
+ try {
+B b;
+throw b;
+ } catch(A) {
+ }
+}
+
+void throw_derived_catch_base_private_ptr() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_private_ptr' which should not throw exceptions
+ try {
+B b;
+throw
+ } catch(A *) {
+ }
+}
+
+void throw_derived_catch_base_protected() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_protected' which should not throw exceptions
+ try {
+C c;
+throw c;
+ } catch(A) {
+ }
+}
+
+void throw_derived_catch_base_protected_ptr() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_protected_ptr' which should not throw exceptions
+ try {
+C c;
+throw
+ } catch(A *) {
+ }
+}
+
+void throw_derived_catch_base_ambiguous() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ambiguous' which should not throw exceptions
+ try {
+E e;
+throw e;
+ } catch(A) {
+ }
+}
+
+void throw_derived_catch_base_ambiguous_ptr() noexcept {
+ // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derived_catch_base_ambiguous_ptr' which should not throw exceptions
+ try {
+E e;
+throw e;
+ } catch(A) {
+ }
+}
+
void try_nested_try(int n) noexcept {
// CHECK-MESSAGES-NOT: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'try_nested_try' which should not throw exceptions
try {
Index: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
