[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<FunctionType>());
----------------
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