[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL abandoned this revision.
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/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

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

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

PiotrZSL wrote:
> isuckatcs wrote:
> > isuckatcs wrote:
> > > PiotrZSL wrote:
> > > > isuckatcs wrote:
> > > > > isuckatcs wrote:
> > > > > > Please cover this line with both positive and negative test cases.
> > > > > > 
> > > > > > Also upon looking up both [[ 
> > > > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] 
> > > > > > (C++ 20 draft) and [[ https://github.com/cplusplus/draft/releases | 
> > > > > > N4917]] (C++ 23 draft), they both say for qualification conversion 
> > > > > > that 
> > > > > > 
> > > > > > 
> > > > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > > > 
> > > > > > Why are we not allowing them if the standard is at least C++ 20?
> > > > > Please resolve this thread.
> > > > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > > > throw_derivedfn_catch_basefn.
> > > > Negative is covered by throw_basefn_catch_basefn.
> > > > 
> > > > For members there are tests throw_basem_catch_basem, 
> > > > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > > > this correctly.
> > > > 
> > > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > > standard.
> > > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > > standard.
> > > 
> > > I don't know how to deal with this tbh. In a static analyzer we usually 
> > > want to consider what the standard says and not what a specific compiler 
> > > implementation does, as the compiler can still be buggy, lack the proper 
> > > support for the standard, etc.
> > > 
> > > Maybe we can add a FIXME here that explains, why this check is here and 
> > > that it's the opposite of what the standard says. Then later if it causes 
> > > issues, it will be easy to see why that happens.
> > > 
> > > @xazax.hun do you have a prefered method to deal with these situations in 
> > > the analyzer? If there is one, we could also try it here.
> > @PiotrZSL this is still unanswered. The standard says these are allowed, so 
> > according to the standard we model something wrong here, unless I'm 
> > misunderstanding something.
> If you talk about about '7.3.5 Qualification conversions' then this is only 
> about cv conversion, not about overall conversion from base to derived type, 
> and this is verify by SatisfiesCVRules lambda.
> 
> Without this line, check fails to detect issue in 
> throw_basefn_catch_derivedfn function under C++20.
> 
> Other interesting thing is that even this code:
> ```
>try {
>  throw ::foo;
>} catch(const void(baseMember::*)()) {
>}
> ```
> 
> Causes exception not to be catch, but if you remove const then it's catch.
> Same behavior is in GCC. I do not sure if 7.3.5 even apply to exceptions, and 
> personally I do not care what standard says, but I care how compilers work.
> 
> Right now both GCC and Clang in C++20 and C++17 work in same way, and this 
> ``if`` should be here so check would be align with how compiler works.
> There is
> This is anyway a insignificant scenario that most probably wont be used in 
> field.
> I do not sure if 7.3.5 (7.3.5 Qualification conversions) even apply to 
> exceptions



> [N4917 Post-Summer 2022 C++ working draft] 14.4 Handling an exception
> [...]
> - the handler is of type cv T or const T& where T is a pointer or 
> pointer-to-member type and E is a pointer or pointer-to-member type that can 
> be converted to T by one or more of
>  - a standard pointer conversion (7.3.12) not involving conversions to 
> pointers to private or protected or ambiguous classes
>  - a function pointer conversion (7.3.14)
>  - a qualification conversion (7.3.6), or <-- here it is
> [...]


>  I do not care what standard says, but I care how compilers work.

Then how would you reason about [[ https://godbolt.org/z/oTr3fbG8T | this ]] 
snippet? MSVC detects that the exception won't be caught and reports a warning, 
while gcc and clang fails to do it. In this case different compilers work 
differently. To be fair, the exception should be caught, since the thrown 
object is convertible to the handler (which is probably why gcc and clang don't 
report a warning).

> Without this line, check fails to detect issue in 
> throw_basefn_catch_derivedfn function under C++20.

Then do a proper investigation why that happens instead of adding random lines, 
which contradict what should happen, so that the test can pass. Probably the 
condition is wrong here and we should return `false` if `!isSameP_i` even if 
the standard is C++20 except for some cases with arrays. Look up the C++17 and 
the C++20 standards and compare them.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551776.
PiotrZSL marked an inline comment as done.
PiotrZSL added a comment.

Add throw_basefn_catch_const_basefn test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

Files:
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: { \
 // RUN: bugprone-exception-escape.IgnoredExceptions: 'ignored1,ignored2', \
 // RUN: bugprone-exception-escape.FunctionsThatShouldNotThrow: 'enabled1,enabled2,enabled3' \
 // RUN: }}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -420,6 +419,7 @@
 struct baseMember {
 int *iptr;
 virtual void foo(){};
+void boo(){};
 };
 
 struct derivedMember : baseMember {
@@ -441,6 +441,29 @@
   }
 }
 
+void throw_basefn_catch_const_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basefn_catch_const_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(const void(baseMember::*)()) {
+  }
+}
+
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
+void throw_basefn_via_derivedfn_catch_basefn() noexcept {
+  try {
+throw ::boo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' which should not throw exceptions
   try {
@@ -721,28 +744,3 @@
   test_basic_no_throw();
   test_basic_throw();
 }
-
-namespace PR55143 { namespace PR40583 {
-
-struct test_explicit_throw {
-test_explicit_throw() throw(int) { throw 42; }
-test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
-test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
-~test_explicit_throw() throw(int) { throw 42; }
-};
-
-struct test_implicit_throw {
-test_implicit_throw() { throw 42; }
-test_implicit_throw(const test_implicit_throw&) { throw 42; }
-test_implicit_throw(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
-test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
-test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
-~test_implicit_throw() { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions
-};
-
-}}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
@@ -29,3 +29,28 @@
   sub_throws() throw() : super_throws() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions
 };
+
+namespace PR55143 { namespace PR40583 {
+
+struct test_explicit_throw {
+test_explicit_throw() throw(int) { throw 42; }
+test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
+test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
+~test_explicit_throw() throw(int) { throw 42; }
+};
+
+struct test_implicit_throw {
+test_implicit_throw() { throw 42; }
+

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL marked 2 inline comments as done.
PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

isuckatcs wrote:
> isuckatcs wrote:
> > PiotrZSL wrote:
> > > isuckatcs wrote:
> > > > isuckatcs wrote:
> > > > > Please cover this line with both positive and negative test cases.
> > > > > 
> > > > > Also upon looking up both [[ 
> > > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] 
> > > > > (C++ 20 draft) and [[ https://github.com/cplusplus/draft/releases | 
> > > > > N4917]] (C++ 23 draft), they both say for qualification conversion 
> > > > > that 
> > > > > 
> > > > > 
> > > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > > 
> > > > > Why are we not allowing them if the standard is at least C++ 20?
> > > > Please resolve this thread.
> > > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > > throw_derivedfn_catch_basefn.
> > > Negative is covered by throw_basefn_catch_basefn.
> > > 
> > > For members there are tests throw_basem_catch_basem, 
> > > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > > this correctly.
> > > 
> > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > standard.
> > > Tested this with GCC, and behavior is proper and independent to C++ 
> > > standard.
> > 
> > I don't know how to deal with this tbh. In a static analyzer we usually 
> > want to consider what the standard says and not what a specific compiler 
> > implementation does, as the compiler can still be buggy, lack the proper 
> > support for the standard, etc.
> > 
> > Maybe we can add a FIXME here that explains, why this check is here and 
> > that it's the opposite of what the standard says. Then later if it causes 
> > issues, it will be easy to see why that happens.
> > 
> > @xazax.hun do you have a prefered method to deal with these situations in 
> > the analyzer? If there is one, we could also try it here.
> @PiotrZSL this is still unanswered. The standard says these are allowed, so 
> according to the standard we model something wrong here, unless I'm 
> misunderstanding something.
If you talk about about '7.3.5 Qualification conversions' then this is only 
about cv conversion, not about overall conversion from base to derived type, 
and this is verify by SatisfiesCVRules lambda.

Without this line, check fails to detect issue in throw_basefn_catch_derivedfn 
function under C++20.

Other interesting thing is that even this code:
```
   try {
 throw ::foo;
   } catch(const void(baseMember::*)()) {
   }
```

Causes exception not to be catch, but if you remove const then it's catch.
Same behavior is in GCC. I do not sure if 7.3.5 even apply to exceptions, and 
personally I do not care what standard says, but I care how compilers work.

Right now both GCC and Clang in C++20 and C++17 work in same way, and this 
``if`` should be here so check would be align with how compiler works.
There is
This is anyway a insignificant scenario that most probably wont be used in 
field.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

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

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

isuckatcs wrote:
> PiotrZSL wrote:
> > isuckatcs wrote:
> > > isuckatcs wrote:
> > > > Please cover this line with both positive and negative test cases.
> > > > 
> > > > Also upon looking up both [[ 
> > > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 
> > > > 20 draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] 
> > > > (C++ 23 draft), they both say for qualification conversion that 
> > > > 
> > > > 
> > > > > each P_i is ... pointer to member of class C_i of type, ...
> > > > 
> > > > Why are we not allowing them if the standard is at least C++ 20?
> > > Please resolve this thread.
> > Positive case is tested by throw_basefn_catch_derivedfn test and 
> > throw_derivedfn_catch_basefn.
> > Negative is covered by throw_basefn_catch_basefn.
> > 
> > For members there are tests throw_basem_catch_basem, 
> > throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers 
> > this correctly.
> > 
> > Tested this with GCC, and behavior is proper and independent to C++ 
> > standard.
> > Tested this with GCC, and behavior is proper and independent to C++ 
> > standard.
> 
> I don't know how to deal with this tbh. In a static analyzer we usually want 
> to consider what the standard says and not what a specific compiler 
> implementation does, as the compiler can still be buggy, lack the proper 
> support for the standard, etc.
> 
> Maybe we can add a FIXME here that explains, why this check is here and that 
> it's the opposite of what the standard says. Then later if it causes issues, 
> it will be easy to see why that happens.
> 
> @xazax.hun do you have a prefered method to deal with these situations in the 
> analyzer? If there is one, we could also try it here.
@PiotrZSL this is still unanswered. The standard says these are allowed, so 
according to the standard we model something wrong here, unless I'm 
misunderstanding something.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 551760.
PiotrZSL added a comment.

Rebase + Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

Files:
  clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: { \
 // RUN: bugprone-exception-escape.IgnoredExceptions: 'ignored1,ignored2', \
 // RUN: bugprone-exception-escape.FunctionsThatShouldNotThrow: 'enabled1,enabled2,enabled3' \
 // RUN: }}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -420,6 +419,7 @@
 struct baseMember {
 int *iptr;
 virtual void foo(){};
+void boo(){};
 };
 
 struct derivedMember : baseMember {
@@ -441,6 +441,21 @@
   }
 }
 
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
+void throw_basefn_via_derivedfn_catch_basefn() noexcept {
+  try {
+throw ::boo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' which should not throw exceptions
   try {
@@ -721,28 +736,3 @@
   test_basic_no_throw();
   test_basic_throw();
 }
-
-namespace PR55143 { namespace PR40583 {
-
-struct test_explicit_throw {
-test_explicit_throw() throw(int) { throw 42; }
-test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
-test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
-test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
-~test_explicit_throw() throw(int) { throw 42; }
-};
-
-struct test_implicit_throw {
-test_implicit_throw() { throw 42; }
-test_implicit_throw(const test_implicit_throw&) { throw 42; }
-test_implicit_throw(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
-test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
-test_implicit_throw& operator=(test_implicit_throw&&) { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: an exception may be thrown in function 'operator=' which should not throw exceptions
-~test_implicit_throw() { throw 42; }
-// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function '~test_implicit_throw' which should not throw exceptions
-};
-
-}}
Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape-throw.cpp
@@ -29,3 +29,28 @@
   sub_throws() throw() : super_throws() {}
   // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: an exception may be thrown in function 'sub_throws' which should not throw exceptions
 };
+
+namespace PR55143 { namespace PR40583 {
+
+struct test_explicit_throw {
+test_explicit_throw() throw(int) { throw 42; }
+test_explicit_throw(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw(test_explicit_throw&&) throw(int) { throw 42; }
+test_explicit_throw& operator=(const test_explicit_throw&) throw(int) { throw 42; }
+test_explicit_throw& operator=(test_explicit_throw&&) throw(int) { throw 42; }
+~test_explicit_throw() throw(int) { throw 42; }
+};
+
+struct test_implicit_throw {
+test_implicit_throw() { throw 42; }
+test_implicit_throw(const test_implicit_throw&) { throw 42; }
+test_implicit_throw(test_implicit_throw&&) { throw 42; }
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: an exception may be thrown in function 'test_implicit_throw' which should not throw exceptions
+test_implicit_throw& operator=(const test_implicit_throw&) { throw 42; }
+

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

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

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

PiotrZSL wrote:
> isuckatcs wrote:
> > isuckatcs wrote:
> > > Please cover this line with both positive and negative test cases.
> > > 
> > > Also upon looking up both [[ 
> > > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 20 
> > > draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] (C++ 
> > > 23 draft), they both say for qualification conversion that 
> > > 
> > > 
> > > > each P_i is ... pointer to member of class C_i of type, ...
> > > 
> > > Why are we not allowing them if the standard is at least C++ 20?
> > Please resolve this thread.
> Positive case is tested by throw_basefn_catch_derivedfn test and 
> throw_derivedfn_catch_basefn.
> Negative is covered by throw_basefn_catch_basefn.
> 
> For members there are tests throw_basem_catch_basem, 
> throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers this 
> correctly.
> 
> Tested this with GCC, and behavior is proper and independent to C++ standard.
> Tested this with GCC, and behavior is proper and independent to C++ standard.

I don't know how to deal with this tbh. In a static analyzer we usually want to 
consider what the standard says and not what a specific compiler implementation 
does, as the compiler can still be buggy, lack the proper support for the 
standard, etc.

Maybe we can add a FIXME here that explains, why this check is here and that 
it's the opposite of what the standard says. Then later if it causes issues, it 
will be easy to see why that happens.

@xazax.hun do you have a prefered method to deal with these situations in the 
analyzer? If there is one, we could also try it here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

isuckatcs wrote:
> isuckatcs wrote:
> > Please cover this line with both positive and negative test cases.
> > 
> > Also upon looking up both [[ 
> > https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 20 
> > draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] (C++ 23 
> > draft), they both say for qualification conversion that 
> > 
> > 
> > > each P_i is ... pointer to member of class C_i of type, ...
> > 
> > Why are we not allowing them if the standard is at least C++ 20?
> Please resolve this thread.
Positive case is tested by throw_basefn_catch_derivedfn test and 
throw_derivedfn_catch_basefn.
Negative is covered by throw_basefn_catch_basefn.

For members there are tests throw_basem_catch_basem, 
throw_basem_catch_derivedm, throw_derivedm_catch_basem that also covers this 
correctly.

Tested this with GCC, and behavior is proper and independent to C++ standard.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 530306.
PiotrZSL marked 2 inline comments as done.
PiotrZSL added a comment.

Add one test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -152,7 +151,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 {
@@ -249,7 +248,7 @@
 void throw_derived_catch_base_ptr_c() noexcept {
   try {
 derived d;
-throw  
+throw 
   } catch(const base *) {
   }
 }
@@ -259,7 +258,7 @@
   try {
 derived d;
 const derived *p = 
-throw p; 
+throw p;
   } catch(base *) {
   }
 }
@@ -282,7 +281,7 @@
   // 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; 
+throw b;
   } catch(A) {
   }
 }
@@ -291,7 +290,7 @@
   // 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  
+throw 
   } catch(A *) {
   }
 }
@@ -300,7 +299,7 @@
   // 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; 
+throw c;
   } catch(A) {
   }
 }
@@ -309,7 +308,7 @@
   // 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  
+throw 
   } catch(A *) {
   }
 }
@@ -318,7 +317,7 @@
   // 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; 
+throw e;
   } catch(A) {
   }
 }
@@ -327,7 +326,7 @@
   // 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; 
+throw e;
   } catch(A) {
   }
 }
@@ -420,6 +419,7 @@
 struct baseMember {
 int *iptr;
 virtual void foo(){};
+void boo(){};
 };
 
 struct derivedMember : baseMember {
@@ -441,6 +441,21 @@
   }
 }
 
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
+void throw_basefn_via_derivedfn_catch_basefn() noexcept {
+  try {
+throw ::boo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' 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
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "ExceptionAnalyzer.h"
+#include 
 
 namespace clang::tidy::utils {
 
@@ -149,37 +150,27 @@
 }
 
 bool isFunctionPointerConvertible(QualType From, QualType To) {
-  if (!From->isFunctionPointerType() && !From->isFunctionType() &&
-  !From->isMemberFunctionPointerType())
-return false;
-
-  if (!To->isFunctionPointerType() && !To->isMemberFunctionPointerType())
+  ODRHash FromHash, ToHash;
+
+  if 

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

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

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

isuckatcs wrote:
> Please cover this line with both positive and negative test cases.
> 
> Also upon looking up both [[ 
> https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 20 
> draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] (C++ 23 
> draft), they both say for qualification conversion that 
> 
> 
> > each P_i is ... pointer to member of class C_i of type, ...
> 
> Why are we not allowing them if the standard is at least C++ 20?
Please resolve this thread.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 524820.
PiotrZSL added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -152,7 +151,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 {
@@ -249,7 +248,7 @@
 void throw_derived_catch_base_ptr_c() noexcept {
   try {
 derived d;
-throw  
+throw 
   } catch(const base *) {
   }
 }
@@ -259,7 +258,7 @@
   try {
 derived d;
 const derived *p = 
-throw p; 
+throw p;
   } catch(base *) {
   }
 }
@@ -282,7 +281,7 @@
   // 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; 
+throw b;
   } catch(A) {
   }
 }
@@ -291,7 +290,7 @@
   // 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  
+throw 
   } catch(A *) {
   }
 }
@@ -300,7 +299,7 @@
   // 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; 
+throw c;
   } catch(A) {
   }
 }
@@ -309,7 +308,7 @@
   // 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  
+throw 
   } catch(A *) {
   }
 }
@@ -318,7 +317,7 @@
   // 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; 
+throw e;
   } catch(A) {
   }
 }
@@ -327,7 +326,7 @@
   // 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; 
+throw e;
   } catch(A) {
   }
 }
@@ -441,6 +440,14 @@
   }
 }
 
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' 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
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "ExceptionAnalyzer.h"
+#include 
 
 namespace clang::tidy::utils {
 
@@ -149,37 +150,27 @@
 }
 
 bool isFunctionPointerConvertible(QualType From, QualType To) {
-  if (!From->isFunctionPointerType() && !From->isFunctionType() &&
-  !From->isMemberFunctionPointerType())
-return false;
-
-  if (!To->isFunctionPointerType() && !To->isMemberFunctionPointerType())
+  ODRHash FromHash, ToHash;
+
+  if (From->isFunctionPointerType())
+FromHash.AddType(From->getPointeeType().getTypePtr());
+  else if (From->isFunctionType())
+FromHash.AddType(From->castAs());
+  else if (From->isMemberFunctionPointerType())
+FromHash.AddType(From->castAs());
+  else
 return false;
 
-  if (To->isFunctionPointerType()) {
-if 

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 524817.
PiotrZSL edited the summary of this revision.
PiotrZSL added a comment.

Update


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -152,7 +151,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 {
@@ -249,7 +248,7 @@
 void throw_derived_catch_base_ptr_c() noexcept {
   try {
 derived d;
-throw  
+throw 
   } catch(const base *) {
   }
 }
@@ -259,7 +258,7 @@
   try {
 derived d;
 const derived *p = 
-throw p; 
+throw p;
   } catch(base *) {
   }
 }
@@ -282,7 +281,7 @@
   // 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; 
+throw b;
   } catch(A) {
   }
 }
@@ -291,7 +290,7 @@
   // 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  
+throw 
   } catch(A *) {
   }
 }
@@ -300,7 +299,7 @@
   // 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; 
+throw c;
   } catch(A) {
   }
 }
@@ -309,7 +308,7 @@
   // 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  
+throw 
   } catch(A *) {
   }
 }
@@ -318,7 +317,7 @@
   // 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; 
+throw e;
   } catch(A) {
   }
 }
@@ -327,7 +326,7 @@
   // 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; 
+throw e;
   } catch(A) {
   }
 }
@@ -420,6 +419,7 @@
 struct baseMember {
 int *iptr;
 virtual void foo(){};
+void cfoo() {};
 };
 
 struct derivedMember : baseMember {
@@ -441,6 +441,14 @@
   }
 }
 
+void throw_derivedfn_catch_basefn() noexcept {
+  // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_derivedfn_catch_basefn' which should not throw exceptions
+  try {
+throw ::foo;
+  } catch(void(baseMember::*)()) {
+  }
+}
+
 void throw_basem_catch_basem_throw() noexcept {
   // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: an exception may be thrown in function 'throw_basem_catch_basem_throw' 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
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "ExceptionAnalyzer.h"
+#include 
 
 namespace clang::tidy::utils {
 
@@ -149,37 +150,27 @@
 }
 
 bool isFunctionPointerConvertible(QualType From, QualType To) {
-  if (!From->isFunctionPointerType() && !From->isFunctionType() &&
-  !From->isMemberFunctionPointerType())
-return false;
-
-  if (!To->isFunctionPointerType() && !To->isMemberFunctionPointerType())
+  ODRHash FromHash, ToHash;
+
+  if (From->isFunctionPointerType())
+FromHash.AddType(From->getPointeeType().getTypePtr());
+  else if (From->isFunctionType())
+

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

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

isuckatcs added a comment.

Please cover the changes in as much test cases as possible.




Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:151
 
+bool isFunctionTypeEqual(const FunctionType *From, const FunctionType *To) {
+  if (From == To)

The naming suggests that we check for equality, however in reality we check for 
convertability here. For example if one of the prototypes has exception spec 
and the other doesn't have one, this can still return true. 

Also instead of manually matching everything, you could take a look at 
`ODRHash` or `StructuralEquivalenceContext`. `ODRHash` for example doesn't take 
the `FunctionProtoType` into account when it generates the hashes AFAIK, so 
maybe you could use it to avoid these manual checks. I don't know if 
`StructuralEquivalenceContext` uses it or not, but you might consider taking a 
look at that too.

Maybe you can also consider pasting the appropriate section of the standard 
here, so that whenever someone reads the code they'll understand why are we 
doing what we're doing.

And please cover this function in positive and negative test cases too.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:185
 if (From->isFunctionType())
-  return To->getPointeeType() == From;
-
-return false;
-  }
-
-  if (To->isMemberFunctionPointerType()) {
-if (!From->isMemberFunctionPointerType())
-  return false;
-
+  return isFunctionTypeEqual(ToFunctionTypePtr,
+ From->castAs());

I think we want to call this function for member function pointers too.

The[[ https://github.com/cplusplus/draft/releases |  C++ N4917 draft ]] says 
the following:
```
7.3.14 Function pointer conversions [conv.fctptr]
A prvalue of type “pointer to noexcept function” can be converted to a prvalue 
of type “pointer to function”.
The result is a pointer to the function. A prvalue of type “pointer to member 
of type noexcept function” can
be converted to a prvalue of type “pointer to member of type function”. The 
result designates the member
function.
```



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:306
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;

Please cover this line with both positive and negative test cases.

Also upon looking up both [[ 
https://www.open-std.org/jtc1/sc22/wg21/docs/standards | N4849 ]] (C++ 20 
draft) and [[ https://github.com/cplusplus/draft/releases | N4917]] (C++ 23 
draft), they both say for qualification conversion that 


> each P_i is ... pointer to member of class C_i of type, ...

Why are we not allowing them if the standard is at least C++ 20?



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:160-162
+// FIXME: Two function pointers can differ in 'noexcept', but they still
+// should be considered to be same, now this triggers false-positive 
because
+// Type* != Type*.

PiotrZSL wrote:
> isuckatcs wrote:
> > Are you sure `noexcept` is stored in the type? The test case you modified 
> > `throw_noexcept_catch_regular()` tests this scenario and in that case the 
> > types seem to be the same even though one of the is noexcept an the other 
> > is not.
> > 
> > If the FIXME is valid the proper way would be to implement it in this patch.
> Yes I'm sure.
> C++11 - no warning
> C++14 - no warning
> C++17 - warning
> Looks like since C++17 noexcept is part of type.
> Initially I wanted only to enable tests under C++17/20.
> That's why "FIXME". I will look into this.
I did some investigation regarding this and if I dump the type of `void 
no_throw() noexcept` both with `-std=c++11` and `-std=c++17` I get the same 
result.
```
FunctionProtoType 'void (void) noexcept' cdecl
`-BuiltinType 'void'
```

I agree however that function pointer conversion was not properly implemented.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 518357.
PiotrZSL added a comment.

Remove unused include


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
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
@@ -148,28 +148,44 @@
   return false;
 }
 
+bool isFunctionTypeEqual(const FunctionType *From, const FunctionType *To) {
+  if (From == To)
+return true;
+
+  const auto *FromProto = dyn_cast(From);
+  const auto *ToProto = dyn_cast(To);
+  if (!FromProto || !ToProto)
+return false;
+
+  if (!FromProto->hasExceptionSpec() && !ToProto->hasExceptionSpec())
+return false;
+
+  return FromProto->getNumParams() == ToProto->getNumParams() &&
+ FromProto->isVariadic() == ToProto->isVariadic() &&
+ FromProto->getParamTypes() == ToProto->getParamTypes() &&
+ FromProto->getReturnType() == ToProto->getReturnType() &&
+ FromProto->getMethodQuals() == ToProto->getMethodQuals() &&
+ FromProto->getRefQualifier() == ToProto->getRefQualifier();
+}
+
 bool isFunctionPointerConvertible(QualType From, QualType To) {
   if (!From->isFunctionPointerType() && !From->isFunctionType() &&
   !From->isMemberFunctionPointerType())
 return false;
 
-  if (!To->isFunctionPointerType() && !To->isMemberFunctionPointerType())
-return false;
-
   if (To->isFunctionPointerType()) {
+const auto *ToFunctionTypePtr =
+To->getPointeeType()->castAs();
+
 if (From->isFunctionPointerType())
-  return To->getPointeeType() == From->getPointeeType();
+  return isFunctionTypeEqual(
+  ToFunctionTypePtr, From->getPointeeType()->castAs());
 
 if (From->isFunctionType())
-  return To->getPointeeType() == From;
-
-return false;
-  }
-
-  if (To->isMemberFunctionPointerType()) {
-if (!From->isMemberFunctionPointerType())
-  return false;
-
+  return isFunctionTypeEqual(ToFunctionTypePtr,
+ From->castAs());
+  } else if (To->isMemberFunctionPointerType() &&
+ From->isMemberFunctionPointerType()) {
 const auto *FromMember = cast(From);
 const auto *ToMember = cast(To);
 
@@ -278,16 +294,17 @@
   return false;
 
 if (!isSameP_i(From, To)) {
-  if (LangOpts.CPlusPlus20) {
-if (From->isConstantArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if (!LangOpts.CPlusPlus20)
+return false;
 
-if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if (From->isConstantArrayType() && !To->isIncompleteArrayType())
+return false;
 
-  } else {
+  if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
+return false;
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;
-  }
 }
 
 ++I;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL updated this revision to Diff 518356.
PiotrZSL marked 2 inline comments as done.
PiotrZSL added a comment.

Update, added support for comparing function pointers without
exception specification.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
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
@@ -7,6 +7,7 @@
 //===--===//
 
 #include "ExceptionAnalyzer.h"
+#include 
 
 namespace clang::tidy::utils {
 
@@ -148,28 +149,44 @@
   return false;
 }
 
+bool isFunctionTypeEqual(const FunctionType *From, const FunctionType *To) {
+  if (From == To)
+return true;
+
+  const auto *FromProto = dyn_cast(From);
+  const auto *ToProto = dyn_cast(To);
+  if (!FromProto || !ToProto)
+return false;
+
+  if (!FromProto->hasExceptionSpec() && !ToProto->hasExceptionSpec())
+return false;
+
+  return FromProto->getNumParams() == ToProto->getNumParams() &&
+ FromProto->isVariadic() == ToProto->isVariadic() &&
+ FromProto->getParamTypes() == ToProto->getParamTypes() &&
+ FromProto->getReturnType() == ToProto->getReturnType() &&
+ FromProto->getMethodQuals() == ToProto->getMethodQuals() &&
+ FromProto->getRefQualifier() == ToProto->getRefQualifier();
+}
+
 bool isFunctionPointerConvertible(QualType From, QualType To) {
   if (!From->isFunctionPointerType() && !From->isFunctionType() &&
   !From->isMemberFunctionPointerType())
 return false;
 
-  if (!To->isFunctionPointerType() && !To->isMemberFunctionPointerType())
-return false;
-
   if (To->isFunctionPointerType()) {
+const auto *ToFunctionTypePtr =
+To->getPointeeType()->castAs();
+
 if (From->isFunctionPointerType())
-  return To->getPointeeType() == From->getPointeeType();
+  return isFunctionTypeEqual(
+  ToFunctionTypePtr, From->getPointeeType()->castAs());
 
 if (From->isFunctionType())
-  return To->getPointeeType() == From;
-
-return false;
-  }
-
-  if (To->isMemberFunctionPointerType()) {
-if (!From->isMemberFunctionPointerType())
-  return false;
-
+  return isFunctionTypeEqual(ToFunctionTypePtr,
+ From->castAs());
+  } else if (To->isMemberFunctionPointerType() &&
+ From->isMemberFunctionPointerType()) {
 const auto *FromMember = cast(From);
 const auto *ToMember = cast(To);
 
@@ -278,16 +295,17 @@
   return false;
 
 if (!isSameP_i(From, To)) {
-  if (LangOpts.CPlusPlus20) {
-if (From->isConstantArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if (!LangOpts.CPlusPlus20)
+return false;
 
-if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if (From->isConstantArrayType() && !To->isIncompleteArrayType())
+return false;
 
-  } else {
+  if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
+return false;
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;
-  }
 }
 
 ++I;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL planned changes to this revision.
PiotrZSL added a comment.

TODO: Fix function pointer compare


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:160-162
+// FIXME: Two function pointers can differ in 'noexcept', but they still
+// should be considered to be same, now this triggers false-positive 
because
+// Type* != Type*.

isuckatcs wrote:
> Are you sure `noexcept` is stored in the type? The test case you modified 
> `throw_noexcept_catch_regular()` tests this scenario and in that case the 
> types seem to be the same even though one of the is noexcept an the other is 
> not.
> 
> If the FIXME is valid the proper way would be to implement it in this patch.
Yes I'm sure.
C++11 - no warning
C++14 - no warning
C++17 - warning
Looks like since C++17 noexcept is part of type.
Initially I wanted only to enable tests under C++17/20.
That's why "FIXME". I will look into this.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:414
   throw 
-} catch(int (*)()) {
+} catch(int (*)() noexcept) {
 }

isuckatcs wrote:
> The name of the function suggests that we throw a noexcept function pointer 
> and catch a non-noexcept one. Please don't change it.
Ack.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

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

isuckatcs added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/ExceptionAnalyzer.cpp:160-162
+// FIXME: Two function pointers can differ in 'noexcept', but they still
+// should be considered to be same, now this triggers false-positive 
because
+// Type* != Type*.

Are you sure `noexcept` is stored in the type? The test case you modified 
`throw_noexcept_catch_regular()` tests this scenario and in that case the types 
seem to be the same even though one of the is noexcept an the other is not.

If the FIXME is valid the proper way would be to implement it in this patch.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone/exception-escape.cpp:414
   throw 
-} catch(int (*)()) {
+} catch(int (*)() noexcept) {
 }

The name of the function suggests that we throw a noexcept function pointer and 
catch a non-noexcept one. Please don't change it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148461/new/

https://reviews.llvm.org/D148461

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D148461: [clang-tidy] Support C++17/20 in bugprone-exception-escape

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL created this revision.
PiotrZSL added reviewers: njames93, carlosgalvezp.
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.

Added support for C++17 or later in tests
Pointers to member functions are now handled correctly in C++20

Depends on D148458 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D148461

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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t 
-- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 
'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, 
value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -412,7 +411,7 @@
   void throw_noexcept_catch_regular() noexcept {
 try {
   throw 
-} catch(int (*)()) {
+} catch(int (*)() noexcept) {
 }
   }
 }
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
@@ -157,6 +157,9 @@
 return false;
 
   if (To->isFunctionPointerType()) {
+// FIXME: Two function pointers can differ in 'noexcept', but they still
+// should be considered to be same, now this triggers false-positive 
because
+// Type* != Type*.
 if (From->isFunctionPointerType())
   return To->getPointeeType() == From->getPointeeType();
 
@@ -278,16 +281,17 @@
   return false;
 
 if (!isSameP_i(From, To)) {
-  if (LangOpts.CPlusPlus20) {
-if (From->isConstantArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if (!LangOpts.CPlusPlus20)
+return false;
 
-if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if (From->isConstantArrayType() && !To->isIncompleteArrayType())
+return false;
 
-  } else {
+  if (From->isIncompleteArrayType() && !To->isIncompleteArrayType())
+return false;
+
+  if (From->isMemberPointerType() || To->isMemberPointerType())
 return false;
-  }
 }
 
 ++I;


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
@@ -1,10 +1,9 @@
-// RUN: %check_clang_tidy -std=c++11,c++14 %s bugprone-exception-escape %t -- \
+// RUN: %check_clang_tidy -std=c++11-or-later %s bugprone-exception-escape %t -- \
 // RUN: -config="{CheckOptions: [ \
 // RUN: {key: bugprone-exception-escape.IgnoredExceptions, value: 'ignored1,ignored2'}, \
 // RUN: {key: bugprone-exception-escape.FunctionsThatShouldNotThrow, value: 'enabled1,enabled2,enabled3'} \
 // RUN: ]}" \
 // RUN: -- -fexceptions
-// FIXME: Fix the checker to work in C++17 or later mode.
 
 struct throwing_destructor {
   ~throwing_destructor() {
@@ -412,7 +411,7 @@
   void throw_noexcept_catch_regular() noexcept {
 try {
   throw 
-} catch(int (*)()) {
+} catch(int (*)() noexcept) {
 }
   }
 }
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
@@ -157,6 +157,9 @@
 return false;
 
   if (To->isFunctionPointerType()) {
+// FIXME: Two function pointers can differ in 'noexcept', but they still
+// should be considered to be same, now this triggers false-positive because
+// Type* != Type*.
 if (From->isFunctionPointerType())
   return To->getPointeeType() == From->getPointeeType();
 
@@ -278,16 +281,17 @@
   return false;
 
 if (!isSameP_i(From, To)) {
-  if (LangOpts.CPlusPlus20) {
-if (From->isConstantArrayType() && !To->isIncompleteArrayType())
-  return false;
+  if