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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
