[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 492115.
isuckatcs added a comment.

- Reverted the changes related to clang itself
- Added more checks for exception handling
- `isQualificationConvertiblePointer` is now iterative, not recursive
- Added more test cases


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 

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added a comment.

This patch got a little bit out of control at the last revision, so I decided 
to remove every change from clang and add everything to the `ExceptionAnalyzer` 
only.
The reason for that is with exceptions we have less conversions to check than 
we have inside the compiler, which can lead to confusion.

For example:

  class A {};
  class B : public A {};
  
  int A::* pa;
  int B::* pb = pa; <-- valid outside of exception handler, invalid in 
exception handler

We can have the conversion `int B::* pb = pa;` because of `7.3.12 
Pointer-to-member conversions`, which is by standard not performed when an 
exception needs to be caught.
See godbolt . (MSVC does catch `A::*` with a 
`B::*` handler for some reason, maybe I miss some flag)

For the above reason, sadly we can't test the changes the way you suggested 
@xazax.hun, like checking if the assigment compiles or not.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added a comment.

> E.g., instead of asserting true/false, checking if the assignment would 
> compile.

This is actually biased. If the result of the compilation is different from the 
result we get from this function, it can also mean a bug in the compiler.

Take a look at this example on godbolt . The 
snippet here is only valid from C++20, but GCC compiles it even in C++17.




Comment at: clang/include/clang/AST/ASTContext.h:2828
 
+  static bool isQualificationConvertiblePointer(QualType From, QualType To,
+LangOptions LangOpts,

erichkeane wrote:
> Please document what this is doing...
This was actually documented, but at the definition. My bad.



Comment at: clang/include/clang/AST/ASTContext.h:2829
+  static bool isQualificationConvertiblePointer(QualType From, QualType To,
+LangOptions LangOpts,
+unsigned CurrentLevel = 0,

erichkeane wrote:
> Why is this static if you need lang-opts?  This should be retrieved by the 
> ASTContext itself.
By taking the language options externally, we might be able to produce more 
descriptive warning messages in the future. 
E.g.: `This conversion is only valid from C++20`, etc.

Also calling this function doesn't depend on `ASTContext` any other way, so it 
can be called even if we don't have access to the `ASTContext` for some reason.

I don't know however if it makes sense to worry about these uses at all.




Comment at: clang/include/clang/AST/ASTContext.h:2831
+unsigned CurrentLevel = 0,
+bool IsToConstSoFar = false);
+

erichkeane wrote:
> What does this name mean here?  It isn't clear to me.
This is documented at the use site of this variable. I couldn't come up with a 
better name for it.
```lang=c++
  // If at the current level To is more cv-qualified than From [...],
  // then there must be a 'const' at every single level (other than level zero)
  // of To up until the current level
  bool MoreCVQualified =
  To.getQualifiers().isStrictSupersetOf(From.getQualifiers());
  if (MoreCVQualified)
Convertible &= IsToConstSoFar;
```



Comment at: clang/include/clang/AST/Type.h:7364
+  else if (isArrayType())
+return getAsArrayTypeUnsafe()->getElementType();
+

erichkeane wrote:
> This changes the meaning here, and this is a commonly used thing.  Why are 
> you doing this?
I missed that it changes the meaning. Though I need this use case, so I 
reverted these changes and created a static global function instead.



Comment at: clang/lib/AST/ASTContext.cpp:13465
+//
+// The function should only be called in C++ mode.
+bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To,

erichkeane wrote:
> Perhaps this should be asserted on!
I personally don't want to assert it, as it won't crash in C mode, it's just 
the fact that some rules here are different in C.
E.g.  (from [[ 
https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
 | cppreference ]]):
```lang=c++
char** p = 0;
const char* const * p2 = p; // error in C, OK in C++
```

(The example above is checked properly but I didn't dig deeper into the other C 
rules, that's why I said that it shouldn't be called)



Comment at: clang/lib/AST/ASTContext.cpp:13466
+// The function should only be called in C++ mode.
+bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To,
+   LangOptions LangOpts,

erichkeane wrote:
> I find myself shocked we don't have something like this already, but what do 
> we mean by 'qualification convertible'?  Is that a term of art I'm missing?
> 
> 
I didn't come up with this name. It is what this conversion is called by the 
standard.

It is §7.3.5 in [[ 
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2020/n4849.pdf | N4849 ]] 
and you can also find it under the same name on [[ 
https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
 | cppreference ]].





Comment at: clang/lib/AST/ASTContext.cpp:13485
+
+if (!To->isPointerType() ||
+!(From->canDecayToPointerType() || From->isPointerType()))

erichkeane wrote:
> I would expect at least the 'to' here to assert as well.  Passing a 'not 
> pointer' as the 'two' when youre testing 'convertible pointer' is odd and a 
> mistake?
Well, technically `MemberPointerType` is also accepted and it's not derived 
from `PointerType`. Though I added an assertion here, but I left the check so 
that we don't crash in release mode.



Comment at: 

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 491910.
isuckatcs marked 9 inline comments as done.
isuckatcs added a comment.

Addressed comments
Updated tests


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
  clang/include/clang/AST/ASTContext.h
  clang/lib/AST/ASTContext.cpp
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/PointerConversionTest.cpp

Index: clang/unittests/AST/PointerConversionTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/PointerConversionTest.cpp
@@ -0,0 +1,285 @@
+//===- unittests/AST/PointerConversionTest.cpp - Pointer conversion tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for ASTContext::isQualificationConvertiblePointer(),
+// which checks if one pointer can be converted to the other.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+#include "clang/Frontend/FrontendActions.h"
+
+namespace clang {
+
+auto DeclMatcher = ast_matchers::declaratorDecl(ast_matchers::unless(ast_matchers::parmVarDecl())).bind("id");
+
+class TypeExtractor : public ast_matchers::MatchFinder::MatchCallback {
+  int MatchCount = 0;
+
+public:
+  QualType T1, T2;
+
+  void run(const ast_matchers::MatchFinder::MatchResult ) override {
+if (const auto *VD = Result.Nodes.getNodeAs("id")) {
+  if (MatchCount == 0)
+T1 = VD->getType();
+  else {
+assert(VD->hasInit() && "Second declaration must be initialized because of compiler error messages.");
+T2 = VD->getType();
+  }
+
+  ++MatchCount;
+} else if (const auto *FD =
+   Result.Nodes.getNodeAs("id")) {
+  if (MatchCount == 0)
+T1 = FD->getType();
+  else
+T2 = FD->getType();
+
+  ++MatchCount;
+}
+  }
+};
+
+void prepareArgAndLangOptionsFromStd(LangStandard Std, std::string , LangOptions ) {
+  Arg = "-std=";
+  Arg += Std.getName();
+
+  std::vector Tmp;
+  LangOptions::setLangDefaults(Opts, Std.Language, {}, Tmp,
+   Std.getLangKind(Std.getName()));
+}
+
+bool CheckIfCompiles(llvm::StringRef Source, LangStandard Std) {
+  std::string StdArg;
+  LangOptions Opts;
+  prepareArgAndLangOptionsFromStd(Std, StdArg, Opts);
+
+  return tooling::runToolOnCodeWithArgs(tooling::newFrontendActionFactory().get()->create(), Source, {StdArg});
+}
+
+bool CheckConvertible(llvm::StringRef Source, LangStandard Std) {
+  TypeExtractor Extractor;
+  ast_matchers::MatchFinder Finder;
+
+  Finder.addMatcher(DeclMatcher, );
+  std::unique_ptr Factory(
+  tooling::newFrontendActionFactory());
+
+  std::string StdArg;
+  LangOptions Opts;
+  prepareArgAndLangOptionsFromStd(Std, StdArg, Opts);
+
+  tooling::runToolOnCodeWithArgs(Factory->create(), Source, {StdArg});
+
+  return ASTContext::isQualificationConvertiblePointer(Extractor.T1,
+   Extractor.T2, Opts);
+}
+
+#define TEST_CONVERSION(ASSERTION, SNIPPET, LANG) { const char* Snippet = SNIPPET; \
+  ASSERTION(CheckConvertible(Snippet, LANG));  \
+  ASSERTION(CheckIfCompiles(Snippet, LANG)); }
+
+const LangStandard & StdCXX98 = LangStandard::getLangStandardForKind(LangStandard::lang_cxx98);
+const LangStandard & StdCXX17 = LangStandard::getLangStandardForKind(LangStandard::lang_cxx17);
+const LangStandard & StdCXX20 = LangStandard::getLangStandardForKind(LangStandard::lang_cxx20);
+const LangStandard & StdCXX2b = LangStandard::getLangStandardForKind(LangStandard::lang_cxx2b);
+
+
+TEST(QualificationConversion, builtin) {
+  TEST_CONVERSION(ASSERT_FALSE, R"cpp(char **p; const char **p1 = p;)cpp", StdCXX98);
+}
+
+TEST(QualificationConversion, builtin2) {
+  TEST_CONVERSION(ASSERT_TRUE, R"cpp(char **p; const char* const * p2 = p;)cpp", StdCXX98);
+}
+
+TEST(QualificationConversion, builtin3) {
+  TEST_CONVERSION(ASSERT_TRUE, R"cpp(char **p; volatile char * const * p3 = p;)cpp", StdCXX98);
+}
+
+TEST(QualificationConversion, builtin4) {
+  TEST_CONVERSION(ASSERT_TRUE, R"cpp(char **p; volatile const char* const* p4 = p;)cpp", StdCXX98);
+}
+
+TEST(QualificationConversion, builtin5) {
+  TEST_CONVERSION(ASSERT_TRUE, R"cpp(double *a[2][3]; double const * const (*ap)[3] = a;)cpp", StdCXX98);
+}
+

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Not a great reviewer here as I'm not particularly sure what is going on, but I 
'looked at the clang parts.




Comment at: clang/include/clang/AST/ASTContext.h:2828
 
+  static bool isQualificationConvertiblePointer(QualType From, QualType To,
+LangOptions LangOpts,

Please document what this is doing...



Comment at: clang/include/clang/AST/ASTContext.h:2829
+  static bool isQualificationConvertiblePointer(QualType From, QualType To,
+LangOptions LangOpts,
+unsigned CurrentLevel = 0,

Why is this static if you need lang-opts?  This should be retrieved by the 
ASTContext itself.



Comment at: clang/include/clang/AST/ASTContext.h:2831
+unsigned CurrentLevel = 0,
+bool IsToConstSoFar = false);
+

What does this name mean here?  It isn't clear to me.



Comment at: clang/include/clang/AST/Type.h:7364
+  else if (isArrayType())
+return getAsArrayTypeUnsafe()->getElementType();
+

This changes the meaning here, and this is a commonly used thing.  Why are you 
doing this?



Comment at: clang/lib/AST/ASTContext.cpp:13465
+//
+// The function should only be called in C++ mode.
+bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To,

Perhaps this should be asserted on!



Comment at: clang/lib/AST/ASTContext.cpp:13466
+// The function should only be called in C++ mode.
+bool ASTContext::isQualificationConvertiblePointer(QualType From, QualType To,
+   LangOptions LangOpts,

I find myself shocked we don't have something like this already, but what do we 
mean by 'qualification convertible'?  Is that a term of art I'm missing?





Comment at: clang/lib/AST/ASTContext.cpp:13485
+
+if (!To->isPointerType() ||
+!(From->canDecayToPointerType() || From->isPointerType()))

I would expect at least the 'to' here to assert as well.  Passing a 'not 
pointer' as the 'two' when youre testing 'convertible pointer' is odd and a 
mistake?



Comment at: clang/lib/AST/ASTContext.cpp:13489
+
+auto FromPointeeTy = From->getPointeeOrArrayElementQualType();
+auto ToPointeeTy = To->getPointeeOrArrayElementQualType();

debatable whether we can use 'auto' here.  I'd lean toward 'no'.



Comment at: clang/lib/AST/ASTContext.cpp:13492
+
+return isQualificationConvertiblePointer(FromPointeeTy, ToPointeeTy,
+ LangOpts, CurrentLevel + 1, true);

I think the recursion here is making this too complicated iwth the 
"IsConstSoFar" and "Level", I'd probably suggest splitting these tasks up.


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 pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Overall looks good to me, I wonder if the tests could be less manual though. 
E.g., instead of asserting true/false, checking if the assignment would 
compile. This way we can be sure that the method in ASTContext matches the 
behavior of the compiler (and we get notified when the two diverge). If we 
could extract the corresponding code from Sema, it would be even better, but I 
do not insist as that might be a lot of work depending on how it interacts with 
other conversions.


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 pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a reviewer: erichkeane.
xazax.hun added a comment.

Since now the patch touches Clang proper, adding one more reviewer.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 489940.
isuckatcs added a comment.

- Renamed and moved `isMultiLevelConvertiblePointer()` to `ASTContext`
- Added a unit test to test the said function


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
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Type.h
  clang/lib/AST/ASTContext.cpp
  clang/unittests/AST/CMakeLists.txt
  clang/unittests/AST/PointerConversionTest.cpp

Index: clang/unittests/AST/PointerConversionTest.cpp
===
--- /dev/null
+++ clang/unittests/AST/PointerConversionTest.cpp
@@ -0,0 +1,245 @@
+//===- unittests/AST/PointerConversionTest.cpp - Pointer conversion tests -===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+// This file contains tests for ASTContext::isQualificationConvertiblePointer(),
+// which checks if one pointer can be converted to the other.
+//
+//===--===//
+
+#include "gtest/gtest.h"
+
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/Tooling.h"
+
+namespace clang {
+
+auto DeclMatcher = ast_matchers::declaratorDecl().bind("id");
+
+class TypeExtractor : public ast_matchers::MatchFinder::MatchCallback {
+  int MatchCount = 0;
+
+public:
+  QualType T1, T2;
+
+  void run(const ast_matchers::MatchFinder::MatchResult ) override {
+if (const auto *VD = Result.Nodes.getNodeAs("id")) {
+  if (MatchCount == 0)
+T1 = VD->getType();
+  else
+T2 = VD->getType();
+
+  ++MatchCount;
+} else if (const auto *FD =
+   Result.Nodes.getNodeAs("id")) {
+  if (MatchCount == 0)
+T1 = FD->getType();
+  else
+T2 = FD->getType();
+
+  ++MatchCount;
+}
+  }
+};
+
+bool CheckConvertible(llvm::StringRef Source, LangStandard Std) {
+  TypeExtractor Extractor;
+  ast_matchers::MatchFinder Finder;
+
+  Finder.addMatcher(DeclMatcher, );
+  std::unique_ptr Factory(
+  tooling::newFrontendActionFactory());
+
+  std::string StdArg = "-std=";
+  StdArg += Std.getName();
+
+  LangOptions Opts;
+  std::vector Tmp;
+  LangOptions::setLangDefaults(Opts, Std.Language, {}, Tmp,
+   Std.getLangKind(Std.getName()));
+
+  tooling::runToolOnCodeWithArgs(Factory->create(), Source, {StdArg});
+
+  return ASTContext::isQualificationConvertiblePointer(Extractor.T1,
+   Extractor.T2, Opts);
+}
+
+TEST(QualificationConversion, builtin) {
+  ASSERT_FALSE(CheckConvertible(
+  R"cpp(char **p; const char **p1;)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx98)));
+}
+
+TEST(QualificationConversion, builtin2) {
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(char **p; const char* const * p2;)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx98)));
+}
+
+TEST(QualificationConversion, builtin3) {
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(char **p; volatile char * const * p3;)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx98)));
+}
+
+TEST(QualificationConversion, builtin4) {
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(char **p; volatile const char* const* p4;)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx98)));
+}
+
+TEST(QualificationConversion, builtin5) {
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(double *a[2][3]; double const * const (*ap)[3];)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx98)));
+}
+
+TEST(QualificationConversion, builtin6) {
+  ASSERT_FALSE(CheckConvertible(
+  R"cpp(double *a[2][3]; double * const (*ap1)[];)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx17)));
+
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(double *a[2][3]; double * const (*ap1)[];)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx20)));
+
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(double *a[2][3]; double * const (*ap1)[];)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx2b)));
+}
+
+TEST(QualificationConversion, builtin7) {
+  ASSERT_TRUE(CheckConvertible(
+  R"cpp(int arr[3]; int *p;)cpp",
+  LangStandard::getLangStandardForKind(LangStandard::lang_cxx98)));
+}
+
+TEST(QualificationConversion, builtin8) {
+  ASSERT_FALSE(CheckConvertible(
+  R"cpp(int arr[3][3]; int **p;)cpp",
+  

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:81
+  FromPointeeTy->getUnqualifiedDesugaredType();
+  const Type *ToPointeeUQTy = ToPointeeTy->getUnqualifiedDesugaredType();
+

Here I am also wondering if we actually want canonical types.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

isuckatcs wrote:
> xazax.hun wrote:
> > isuckatcs wrote:
> > > xazax.hun wrote:
> > > > If none of the functions I recommended works for you, I still strongly 
> > > > suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` 
> > > > and `UnwrapSimilarArrayTypes`.
> > > I didn't check all of the functions inside `ASTContext`, but here we have 
> > > very specific rules, we need to check. One utility might help with one 
> > > check or two, but won't replace the need to go ove all of them. Also I 
> > > think it's easier to understand which section checks what, if I keep them 
> > > separated.
> > > 
> > > In an ealier comment I link to [[ 
> > > https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
> > >  | the section on cppreference ]], which discusses what these rules are. 
> > > Also there I found one controversial example, that's only detected by 
> > > MSVC. I wonder if you know why that happens.
> > I see. It is unfortunate that we cannot simply reuse the corresponding 
> > functionality from Sema. The code mostly looks good to me, apart from some 
> > nits.
> > It is unfortunate that we cannot simply reuse the corresponding 
> > functionality from Sema.
> 
> It is indeed unfortunate, though I wonder if we want to move this 
> functionality to `ASTContext`, so that it can be reused outside the 
> `ExceptionAnalyzer`.
I am not opposed to that, but I think in that case we want this method to be 
used in the regular compilation pipeline to make sure it is correct. 


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

xazax.hun wrote:
> isuckatcs wrote:
> > xazax.hun wrote:
> > > If none of the functions I recommended works for you, I still strongly 
> > > suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` and 
> > > `UnwrapSimilarArrayTypes`.
> > I didn't check all of the functions inside `ASTContext`, but here we have 
> > very specific rules, we need to check. One utility might help with one 
> > check or two, but won't replace the need to go ove all of them. Also I 
> > think it's easier to understand which section checks what, if I keep them 
> > separated.
> > 
> > In an ealier comment I link to [[ 
> > https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
> >  | the section on cppreference ]], which discusses what these rules are. 
> > Also there I found one controversial example, that's only detected by MSVC. 
> > I wonder if you know why that happens.
> I see. It is unfortunate that we cannot simply reuse the corresponding 
> functionality from Sema. The code mostly looks good to me, apart from some 
> nits.
> It is unfortunate that we cannot simply reuse the corresponding functionality 
> from Sema.

It is indeed unfortunate, though I wonder if we want to move this functionality 
to `ASTContext`, so that it can be reused outside the `ExceptionAnalyzer`.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 489626.
isuckatcs marked 7 inline comments 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,24 @@
   }
 }
 
+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 *) {
+  }
+}
+
 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
===
--- clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
+++ clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.h
@@ -80,7 +80,7 @@
 /// possible to catch multiple exception types by one 'catch' if they
 /// are a subclass of the 'catch'ed exception type.
 /// Returns 'true' if some exceptions were filtered, otherwise 'false'.
-bool filterByCatch(const Type 

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Also, could you open a bug report about the strange exception behavior on 
GitHub? Hopefully someone working on conformance can take a look.


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 pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned CurrentLevel = 0,

isuckatcs wrote:
> xazax.hun wrote:
> > xazax.hun wrote:
> > > Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is 
> > > already doing what you want. 
> > Oh, maybe it is not, but it might also make sense to take a look at 
> > `ASTContext::typesAreCompatible`.
> > Did you take a look at ASTContext::hasCvrSimilarType? I wonder if it is 
> > already doing what you want.
> 
> This function compares the types by removing the CVR qualifiers.
> 
> > Oh, maybe it is not, but it might also make sense to take a look at 
> > ASTContext::typesAreCompatible.
> 
> This one only compares the canonical types if the language is C++.
> 
> ```lang=c++
> if (getLangOpts().CPlusPlus)
> return hasSameType(LHS, RHS);
> 
> bool hasSameType(QualType T1, QualType T2) const {
> return getCanonicalType(T1) == getCanonicalType(T2);
>   }
> 
> ```
Could you rename the arguments like `To`, `From` or `Exception`, `Catch` or 
something to make the direction of the conversion clearer?



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

isuckatcs wrote:
> xazax.hun wrote:
> > If none of the functions I recommended works for you, I still strongly 
> > suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` and 
> > `UnwrapSimilarArrayTypes`.
> I didn't check all of the functions inside `ASTContext`, but here we have 
> very specific rules, we need to check. One utility might help with one check 
> or two, but won't replace the need to go ove all of them. Also I think it's 
> easier to understand which section checks what, if I keep them separated.
> 
> In an ealier comment I link to [[ 
> https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
>  | the section on cppreference ]], which discusses what these rules are. Also 
> there I found one controversial example, that's only detected by MSVC. I 
> wonder if you know why that happens.
I see. It is unfortunate that we cannot simply reuse the corresponding 
functionality from Sema. The code mostly looks good to me, apart from some nits.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:111
+// ... [or there is an array type of known bound in P1 and
+// an array type of unknown bound in P2 (since C++20)] then
+// there must be a 'const' at every single level (other than

If this rule only applies to C++20 and above, consider adding a language 
version check for LangOpts.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:130-131
+  if (!P1->isPointerType() || !P2->isPointerType())
+return convertible && P1->getUnqualifiedDesugaredType() ==
+  P2->getUnqualifiedDesugaredType();
+

Looking at the documentation of DesugaredType:
```
This takes off typedefs, typeof's etc. If the outer level of the type is 
already concrete, it returns it unmodified. This is similar to getting the 
canonical type, but it doesn't remove all typedefs. For example, it returns 
"T*" as "T*", (not as "int*"), because the pointer is concrete.
```
I wonder if we actually want to compare canonical types rather than desugared 
types.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned CurrentLevel = 0,

xazax.hun wrote:
> xazax.hun wrote:
> > Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is 
> > already doing what you want. 
> Oh, maybe it is not, but it might also make sense to take a look at 
> `ASTContext::typesAreCompatible`.
> Did you take a look at ASTContext::hasCvrSimilarType? I wonder if it is 
> already doing what you want.

This function compares the types by removing the CVR qualifiers.

> Oh, maybe it is not, but it might also make sense to take a look at 
> ASTContext::typesAreCompatible.

This one only compares the canonical types if the language is C++.

```lang=c++
if (getLangOpts().CPlusPlus)
return hasSameType(LHS, RHS);

bool hasSameType(QualType T1, QualType T2) const {
return getCanonicalType(T1) == getCanonicalType(T2);
  }

```



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

xazax.hun wrote:
> If none of the functions I recommended works for you, I still strongly 
> suggest reusing utilities from ASTContext, like `UnwrapSimilarTypes` and 
> `UnwrapSimilarArrayTypes`.
I didn't check all of the functions inside `ASTContext`, but here we have very 
specific rules, we need to check. One utility might help with one check or two, 
but won't replace the need to go ove all of them. Also I think it's easier to 
understand which section checks what, if I keep them separated.

In an ealier comment I link to [[ 
https://en.cppreference.com/w/cpp/language/implicit_conversion#Qualification_conversions
 | the section on cppreference ]], which discusses what these rules are. Also 
there I found one controversial example, that's only detected by MSVC. I wonder 
if you know why that happens.


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 pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:95
+
+  if (P1->isArrayType() && P2->isArrayType()) {
+// At every level that array type is involved in, at least

If none of the functions I recommended works for you, I still strongly suggest 
reusing utilities from ASTContext, like `UnwrapSimilarTypes` and 
`UnwrapSimilarArrayTypes`.


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 pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned CurrentLevel = 0,

xazax.hun wrote:
> Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is 
> already doing what you want. 
Oh, maybe it is not, but it might also make sense to take a look at 
`ASTContext::typesAreCompatible`.


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 pointers in `ExceptionAnalyzer`

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:60
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned CurrentLevel = 0,

Did you take a look at `ASTContext::hasCvrSimilarType`? I wonder if it is 
already doing what you want. 


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs added a comment.

I've found a strange scenario.

The following conversions are allowed

  double *a[2][3];
  double const * const (*ap)[3] = a; // OK
  double * const (*ap1)[] = a;   // OK since C++20

However if the same conversion is supposed to be performed in a `catch()` 
statement, it's not happening and the thrown object is not caught.
See it on godbolt .

Quoteing cppreference :

  When an exception is thrown by any statement in compound-statement, the 
exception object of type E is matched against the types of the formal 
parameters T of each catch-clause in handler-seq, in the order in which the 
catch clauses are listed. The exception is a match if any of the following is 
true:
  
  ...
  - T is (possibly cv-qualified) U or const U& (since C++14), and U is a 
pointer or pointer to member type, and E is also a pointer or pointer to member 
type that is implicitly convertible to U by one or more of
- a standard pointer conversion other than one to a private, protected, or 
ambiguous base class
- **a qualification conversion**
- a function pointer conversion (since C++17)
  ...

Checking the quality conversion related part of cppreference 

 lists the examples I quoted before as performable conversions.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 466312.
isuckatcs marked 2 inline comments as done.
isuckatcs added a comment.

Addressed comments and added support for multi-level pointers too.


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/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,24 @@
   }
 }
 
+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 *) {
+  }
+}
+
 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.cpp
===
--- clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
+++ clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -56,11 +56,108 @@
   [BaseClass](const CXXRecordDecl *Cur) { return Cur != BaseClass; });
 }
 
+// Checks if T1 is convertible to T2.
+static bool isMultiLevelConvertiblePointer(QualType P1, QualType P2,
+   unsigned 

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:81
+   isBaseOf(TPointeeUQTy, BPointeeUQTy)) &&
+  (TCVR == 0 || (BCVR ^ TCVR) == 0 || (BCVR & TCVR) > BCVR)) {
+TypesToDelete.push_back(T);

This harms readability and it doesn't cover extended qualifiers. I believe 
Qualifiers::isStrictSuperSet would be a better candidate to use here.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:105
+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 {

These check not lines are useless and should be removed, same goes below.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 466269.
isuckatcs marked an inline comment as done.
isuckatcs added a comment.

Addressed inline comment


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/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,84 @@
   }
 }
 
+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 
+  } catch(const int *) {}
+}
+
+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 
+  } catch(volatile int *) {}
+}
+
+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_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 +190,25 @@
   }
 }
 
+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  
+  } 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 *) {
+  }
+}
+
 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.cpp
===
--- clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
+++ clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -61,6 +61,27 @@
   for (const Type *T : ThrownExceptions) {
 if (T == BaseClass || isBaseOf(T, BaseClass))
   TypesToDelete.push_back(T);
+else if (T->isPointerType() && BaseClass->isPointerType()) {
+  QualType BPointeeTy = BaseClass->getAs()->getPointeeType();
+  QualType TPointeeTy = T->getAs()->getPointeeType();
+
+  const Type *BPointeeUQTy = BPointeeTy->getUnqualifiedDesugaredType();
+  const Type *TPointeeUQTy = TPointeeTy->getUnqualifiedDesugaredType();
+
+  unsigned BCVR = BPointeeTy.getCVRQualifiers();
+  unsigned TCVR = TPointeeTy.getCVRQualifiers();
+
+  // In case the unqualified types are the same, the exception will be
+ 

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:68
+
+  auto BPointeeUQTy = BPointeeTy->getUnqualifiedDesugaredType();
+  auto TPointeeUQTy = TPointeeTy->getUnqualifiedDesugaredType();

Please don't use `auto` unless type is spelled in same statement or iterator. 
Same below.


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 pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs updated this revision to Diff 466204.
isuckatcs edited the summary of this revision.
isuckatcs added a comment.

Updated


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/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,84 @@
   }
 }
 
+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 
+  } catch(const int *) {}
+}
+
+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 
+  } catch(volatile int *) {}
+}
+
+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_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 +190,25 @@
   }
 }
 
+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  
+  } 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 *) {
+  }
+}
+
 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.cpp
===
--- clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
+++ clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -61,6 +61,27 @@
   for (const Type *T : ThrownExceptions) {
 if (T == BaseClass || isBaseOf(T, BaseClass))
   TypesToDelete.push_back(T);
+else if (T->isPointerType() && BaseClass->isPointerType()) {
+  auto BPointeeTy = BaseClass->getAs()->getPointeeType();
+  auto TPointeeTy = T->getAs()->getPointeeType();
+
+  auto BPointeeUQTy = BPointeeTy->getUnqualifiedDesugaredType();
+  auto TPointeeUQTy = TPointeeTy->getUnqualifiedDesugaredType();
+
+  auto BCVR = BPointeeTy.getCVRQualifiers();
+  auto TCVR = TPointeeTy.getCVRQualifiers();
+
+  // In case the unqualified types are the same, the exception will be
+  // caught if
+  //  1.) the thrown 

[PATCH] D135495: [clang-tidy] handle pointers in `ExceptionAnalyzer`

[Domján Dániel via Phabricator via cfe-commits]

isuckatcs created this revision.
isuckatcs added reviewers: njames93, baloghadamsoftware, aaron.ballman, 
LegalizeAdulthood.
Herald added subscribers: carlosgalvezp, manas, ASDenysPetrov, dkrupp, 
donat.nagy, Szelethus, a.sidorin, rnkovacs, xazax.hun.
Herald added a project: All.
isuckatcs requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

`ExceptionAnalyzer` only compared exact types in case of pointer,
which is incorrect. An `int *` can be caught by a `const int *` handler,
but `ExceptionAnalyzer` falsely reported it an escaping exception.

For example:

  void foo() noexcept {
try {
  int a = 1;
  throw 
} catch (const int *) {
}
  }

In function `foo()` the `` is caught by the handler, but clang-tidy 
reports the following warning:

  warning: an exception may be thrown in function 'foo' which should not throw 
exceptions [bugprone-exception-escape]


https://reviews.llvm.org/D135495

Files:
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.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
@@ -101,6 +101,84 @@
   }
 }
 
+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 
+  } catch(const int *) {}
+}
+
+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 
+  } catch(volatile int *) {}
+}
+
+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_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 {};
 
Index: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
===
--- clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
+++ clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp
@@ -61,6 +61,24 @@
   for (const Type *T : ThrownExceptions) {
 if (T == BaseClass || isBaseOf(T, BaseClass))
   TypesToDelete.push_back(T);
+else if (T->isPointerType() && BaseClass->isPointerType()) {
+  auto BPointeeTy = BaseClass->getAs()->getPointeeType();
+  auto TPointeeTy = T->getAs()->getPointeeType();
+
+  auto BCVR = BPointeeTy.getCVRQualifiers();
+  auto TCVR = TPointeeTy.getCVRQualifiers();
+
+  // In case the unqualified types are the same, the exception will be
+  // caught if
+  //  1.) the thrown type doesn't have qualifiers
+  //  2.) the handler has the same qualifiers as the thrown type
+  //  3.) the handle has more qualifiers than the thrown type
+  if