[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 &baseMember::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