[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 &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.
> 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/

https://reviews.llvm.org/D148461